Re: [PATCH v2 05/10] ceph: convert ceph_submit_write() to folios

On 8/6/26 2:58 PM, Tal Zussman wrote:
> On 8/4/26 2:52 PM, Tal Zussman wrote:
>> @@ -1436,14 +1445,14 @@ int ceph_submit_write(struct address_space *mapping,
>>  	struct ceph_client *cl = fsc->client;
>>  	struct ceph_vino vino = ceph_vino(inode);
>>  	struct ceph_osd_request *req = NULL;
>> -	struct page *page = NULL;
>> +	struct folio *folio = NULL;
>>  	bool caching = ceph_is_cache_enabled(inode);
>>  	u64 offset;
>>  	u64 len;
>>  	unsigned i;
>>  
>>  new_request:
>> -	offset = ceph_fscrypt_page_offset(ceph_wbc->pages[0]);
>> +	offset = ceph_fscrypt_folio_offset(page_folio(ceph_wbc->pages[0]));
>>  	len = ceph_wbc->wsize;
>>  
>>  	req = ceph_osdc_new_request(&fsc->client->osdc,
>> @@ -1467,29 +1476,28 @@ int ceph_submit_write(struct address_space *mapping,
>>  		BUG_ON(IS_ERR(req));
>>  	}
>>  
>> -	page = ceph_wbc->pages[ceph_wbc->locked_pages - 1];
>> -	BUG_ON(len < ceph_fscrypt_page_offset(page) + thp_size(page) - offset);
>> +	folio = page_folio(ceph_wbc->pages[ceph_wbc->locked_pages - 1]);
>> +	BUG_ON(len < ceph_fscrypt_folio_offset(folio) + folio_size(folio) - offset);
>>  
>>  	if (!ceph_inc_osd_stopping_blocker(fsc->mdsc)) {
>>  		for (i = 0; i < folio_batch_count(&ceph_wbc->fbatch); i++) {
>> -			struct folio *folio = ceph_wbc->fbatch.folios[i];
>> +			folio = ceph_wbc->fbatch.folios[i];
>>  
>>  			if (!folio)
>>  				continue;
> 
> This, ...
> 
>>  
>> -			page = &folio->page;
>> -			redirty_page_for_writepage(wbc, page);
>> -			unlock_page(page);
>> +			folio_redirty_for_writepage(wbc, folio);
>> +			folio_unlock(folio);
>>  		}
>>  

Actually, Sashiko points out a deeper issue with this loop:

"Could this unconditional folio_unlock() crash the kernel?
Looking at ceph_process_folio_batch(), folios that fail folio_trylock() or
are already under writeback are left in the fbatch array without being locked
by the current thread.
Then ceph_shift_unused_folios_left() shifts these unprocessed, unlocked folios
to the start of the batch.
If ceph_submit_write() subsequently fails the ceph_inc_osd_stopping_blocker()
check, it iterates over all remaining folios in ceph_wbc->fbatch and calls
folio_unlock() on folios we don't hold the lock for."

...along with 5 other pre-existing issues.

Seems like the folio_batch loop here should just be deleted? But that seems
like it should be a separate patch with a Fixes tag...

>>  		for (i = 0; i < ceph_wbc->locked_pages; i++) {
>> -			page = ceph_fscrypt_pagecache_page(ceph_wbc->pages[i]);
>> +			folio = ceph_fscrypt_pagecache_folio(page_folio(ceph_wbc->pages[i]));
>>  
>> -			if (!page)
>> +			if (!folio)
>>  				continue;
>>  
> 
> ... and this check are unnecessary. ceph_fscrypt_pagecache_page() can't
> take or return NULL, and ceph_shift_unused_folios_left() compacts NULLs
> out of ceph_wbc->fbatch. Will remove both in v3.
> 
>> -			redirty_page_for_writepage(wbc, page);
>> -			unlock_page(page);
>> +			folio_redirty_for_writepage(wbc, folio);
>> +			folio_unlock(folio);
>>  		}
>>