Re: [f2fs-dev] [RFC PATCH 3/9] f2fs: support regular file buffered writes on large folios

On 7/10/26 09:15, Nanzhe Zhao wrote:
>> If the function is global, it needs to add f2fs_ prefix, otherwise it will
>> pollute global namespace.
> 
> Got it. Then folio_has_ffs() should also have a f2fs_ prefix, right?
> Hmmmm, then should all previous ffs_* helper functions be called
> f2fs_ffs*, or f2fs_fs*? (f2fs_fs means the abbreviation of

It will be better to clean up w/ that, I guess for previous helpers in
upstream, we can clean up w/ a separated patch.

> f2fs_folio_state.) Or do you have a better naming idea?

I guess you can focus on your newly introduced function name first.

ffs_test_blk_uptodate -> f2fs_ffs_blk_test_uptodate?

> 
>>> +static bool f2fs_find_next_need_read_block(const struct folio *folio,
>>
>> Since it's static, we can get rid of f2fs_ prefix,
>>
>> find_next_invalid_block() will be better? not sure.
> 
> I think just calling it find_next_need_read_block() or
> folio_find_next_need_read_block() is better?

I think "invalid" indicates non-uptodate status of the block, it needs to
be read later.

> 
> Because this function is actually implemented to only read the first
> subpage or the last subpage in the folio which is written by partial
> write and is still not uptodate. The subpages in the middle that are fully
> covered by block-size writes are skipped from read.
> 
> I know prepare_write_begin skips read for fully covered order-0 folio.
> I want to avoid potential performance regression where large folio write_begin
> would read fully-covered middle subpages while the old order-0 folio path would
> skip such reads.
> 
> 
>>> +		if (!f2fs_lookup_read_extent_cache_block(inode, index,
>>> +						 &dn.data_blkaddr)) {
>>> +			if (IS_DEVICE_ALIASING(inode))
>>> +				return -ENODATA;
>>> +
>>> +			set_new_dnode(&dn, inode, NULL, NULL, 0);
>>> +			err = f2fs_get_dnode_of_data(&dn, index, LOOKUP_NODE);
>>> +			get_dn = true;
>>> +			if (err)
>>> +				goto out;
>>
>> Missed to call f2fs_put_dnode(&dn) in the loop?
> 
> Both the error path and the normal path will eventually reach the out label path,
> and out will call f2fs_put_dnode(&dn) if get_dn is true.
> Or am I missing something?

For above case: both head and tail has partial updated data, then we call
f2fs_get_dnode_of_data() twice, but call f2fs_put_dnode() once?

> 
>>> +		err = submit_bio_wait(bio);
>>
>> If we didn't call f2fs_submit_read_bio(), we may miss to handle tracepoint,
>> iostat and blk_crypto stuff?
> 
> Yes, But for large folio I think we need a sync read helper here, instead of
> directly switching to f2fs_submit_read_bio(). The helper should keep the synchronous
> write_begin semantics, while still going through f2fs trace/iostat and
> blk-crypto submission handling. I know iomap use submit_bio_wait for large folio
> buffered write.

Yes, please figure out a helper here to cover all trace/iostat/blk-crypto
functionality.

Thanks,

> 
> Thanks,
> 



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel