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
