Re: [f2fs-dev] [PATCH 6/9] f2fs: implement iomap operations

[Eric Biggers <ebiggers@kernel.org>]

Hi Christoph,

On Mon, Jul 19, 2021 at 10:59:10AM +0200, Christoph Hellwig wrote:
> On Fri, Jul 16, 2021 at 09:39:16AM -0500, Eric Biggers wrote:
> > +static blk_qc_t f2fs_dio_submit_bio(struct inode *inode, struct iomap *iomap,
> > +				    struct bio *bio, loff_t file_offset)
> > +{
> > +	struct f2fs_private_dio *dio;
> > +	bool write = (bio_op(bio) == REQ_OP_WRITE);
> > +
> > +	dio = f2fs_kzalloc(F2FS_I_SB(inode),
> > +			sizeof(struct f2fs_private_dio), GFP_NOFS);
> > +	if (!dio)
> > +		goto out;
> > +
> > +	dio->inode = inode;
> > +	dio->orig_end_io = bio->bi_end_io;
> > +	dio->orig_private = bio->bi_private;
> > +	dio->write = write;
> > +
> > +	bio->bi_end_io = f2fs_dio_end_io;
> > +	bio->bi_private = dio;
> > +
> > +	inc_page_count(F2FS_I_SB(inode),
> > +			write ? F2FS_DIO_WRITE : F2FS_DIO_READ);
> > +
> > +	return submit_bio(bio);
> 
> I don't think there is any need for this mess.  The F2FS_DIO_WRITE /
> F2FS_DIO_READ counts are only used to check if there is any inflight
> I/O at all.  So instead we can increment them once before calling
> iomap_dio_rw, and decrement them in ->end_io or for a failure/noop
> exit from iomap_dio_rw.  Untested patch below.  Note that all this
> would be much simpler to review if the last three patches were folded
> into a single one.
> 

I am trying to do this, but unfortunately I don't see a way to make it work
correctly in all cases.

The main problem is that when iomap_dio_rw() returns an error (other than
-EIOCBQUEUED), there is no way to know whether ->end_io() has been called or
not.  This is because iomap_dio_rw() can fail either early, before "starting"
the I/O (in which case ->end_io() won't have been called), or later, after
"starting" the I/O (in which case ->end_io() will have been called).  Note that
this can't be worked around by checking whether the iov_iter has been advanced
or not, since a failure could occur between "starting" the I/O and the iov_iter
being advanced for the first time.

Would you be receptive to adding a ->begin_io() callback to struct iomap_dio_ops
in order to allow filesystems to maintain counters like this?

Either way, given the problem here, I think I should leave this out of the
initial conversion and just do a dumb translation of the existing f2fs logic to
start with, like I have in this patch.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel