Re: [PATCH v10 02/41] iomap: support REQ_OP_ZONE_APPEND

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Nov 10, 2020 at 10:55:06AM -0800, Darrick J. Wong wrote:
> On Tue, Nov 10, 2020 at 08:26:05PM +0900, Naohiro Aota wrote:
> > A ZONE_APPEND bio must follow hardware restrictions (e.g. not exceeding
> > max_zone_append_sectors) not to be split. bio_iov_iter_get_pages builds
> > such restricted bio using __bio_iov_append_get_pages if bio_op(bio) ==
> > REQ_OP_ZONE_APPEND.
> > 
> > To utilize it, we need to set the bio_op before calling
> > bio_iov_iter_get_pages(). This commit introduces IOMAP_F_ZONE_APPEND, so
> > that iomap user can set the flag to indicate they want REQ_OP_ZONE_APPEND
> > and restricted bio.
> > 
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> >  fs/iomap/direct-io.c  | 41 +++++++++++++++++++++++++++++++++++------
> >  include/linux/iomap.h |  1 +
> >  2 files changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> > index c1aafb2ab990..f04572a55a09 100644
> > --- a/fs/iomap/direct-io.c
> > +++ b/fs/iomap/direct-io.c
> > @@ -200,6 +200,34 @@ iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos,
> >  	iomap_dio_submit_bio(dio, iomap, bio, pos);
> >  }
> >  
> > +/*
> > + * Figure out the bio's operation flags from the dio request, the
> > + * mapping, and whether or not we want FUA.  Note that we can end up
> > + * clearing the WRITE_FUA flag in the dio request.
> > + */
> > +static inline unsigned int
> > +iomap_dio_bio_opflags(struct iomap_dio *dio, struct iomap *iomap, bool use_fua)
> 
> Hmm, just to check my understanding of what iomap has to do to support
> all this:
> 
> When we're wanting to use a ZONE_APPEND command, the @iomap structure
> has to have IOMAP_F_ZONE_APPEND set in iomap->flags, iomap->type is set
> to IOMAP_MAPPED, but what should iomap->addr be set to?
> 
> I gather from what I see in zonefs and the relevant NVME proposal that
> iomap->addr should be set to the (byte) address of the zone we want to
> append to?  And if we do that, then bio->bi_iter.bi_sector will be set
> to sector address of iomap->addr, right?
> 
> (I got lost trying to figure out how btrfs sets ->addr for appends.)
> 
> Then when the IO completes, the block layer sets bio->bi_iter.bi_sector
> to wherever the drive told it that it actually wrote the bio, right?
> 
> If that's true, then that implies that need_zeroout must always be false
> for an append operation, right?  Does that also mean that the directio
> request has to be aligned to an fs block and not just the sector size?
> 
> Can userspace send a directio append that crosses a zone boundary?  If
> so, what happens if a direct append to a lower address fails but a
> direct append to a higher address succeeds?

Bleh, vim tomfoolery == missing sentence.  Change the above paragraph to
read:

Can userspace send a directio append that crosses a zone boundary?  Can
we issue multiple bios for a single append write?  What happens if a
direct append to a lower address fails but a direct append to a higher
address succeeds?

--D

> > +{
> > +	unsigned int opflags = REQ_SYNC | REQ_IDLE;
> > +
> > +	if (!(dio->flags & IOMAP_DIO_WRITE)) {
> > +		WARN_ON_ONCE(iomap->flags & IOMAP_F_ZONE_APPEND);
> > +		return REQ_OP_READ;
> > +	}
> > +
> > +	if (iomap->flags & IOMAP_F_ZONE_APPEND)
> > +		opflags |= REQ_OP_ZONE_APPEND;
> > +	else
> > +		opflags |= REQ_OP_WRITE;
> > +
> > +	if (use_fua)
> > +		opflags |= REQ_FUA;
> > +	else
> > +		dio->flags &= ~IOMAP_DIO_WRITE_FUA;
> > +
> > +	return opflags;
> > +}
> > +
> >  static loff_t
> >  iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
> >  		struct iomap_dio *dio, struct iomap *iomap)
> > @@ -278,6 +306,13 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
> >  		bio->bi_private = dio;
> >  		bio->bi_end_io = iomap_dio_bio_end_io;
> >  
> > +		/*
> > +		 * Set the operation flags early so that bio_iov_iter_get_pages
> > +		 * can set up the page vector appropriately for a ZONE_APPEND
> > +		 * operation.
> > +		 */
> > +		bio->bi_opf = iomap_dio_bio_opflags(dio, iomap, use_fua);
> 
> I'm also vaguely wondering how to communicate the write location back to
> the filesystem when the bio completes?  btrfs handles the bio completion
> completely so it doesn't have a problem, but for other filesystems
> (cough future xfs cough) either we'd have to add a new callback for
> append operations; or I guess everyone could hook the bio endio.
> 
> Admittedly that's not really your problem, and for all I know hch is
> already working on this.
> 
> --D
> 
> > +
> >  		ret = bio_iov_iter_get_pages(bio, dio->submit.iter);
> >  		if (unlikely(ret)) {
> >  			/*
> > @@ -292,14 +327,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length,
> >  
> >  		n = bio->bi_iter.bi_size;
> >  		if (dio->flags & IOMAP_DIO_WRITE) {
> > -			bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_IDLE;
> > -			if (use_fua)
> > -				bio->bi_opf |= REQ_FUA;
> > -			else
> > -				dio->flags &= ~IOMAP_DIO_WRITE_FUA;
> >  			task_io_account_write(n);
> >  		} else {
> > -			bio->bi_opf = REQ_OP_READ;
> >  			if (dio->flags & IOMAP_DIO_DIRTY)
> >  				bio_set_pages_dirty(bio);
> >  		}
> > diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> > index 4d1d3c3469e9..1bccd1880d0d 100644
> > --- a/include/linux/iomap.h
> > +++ b/include/linux/iomap.h
> > @@ -54,6 +54,7 @@ struct vm_fault;
> >  #define IOMAP_F_SHARED		0x04
> >  #define IOMAP_F_MERGED		0x08
> >  #define IOMAP_F_BUFFER_HEAD	0x10
> > +#define IOMAP_F_ZONE_APPEND	0x20
> >  
> >  /*
> >   * Flags set by the core iomap code during operations:
> > -- 
> > 2.27.0
> >