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

> 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
f2fs_folio_state.) Or do you have a better naming idea?

> > +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?

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?

> > +		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.

Thanks,



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