On 29 June 2018 at 10:56, Christoph Hellwig <hch@xxxxxx> wrote: > This looks generally fine. But I think it might be worth refactoring > iomap_dio_actor a bit first, e.g. something like this new patch > before yours, which would also nicely solve your alignmnet concern > (entirely untested for now): This looks correct. I've rebased my patches on top of it and I ran the xfstest auto group on gfs2 and xfs on top. Can you push this to your gfs2-iomap branch? I'll then repost an updated version of "iomap: Direct I/O for inline data". > --- > From f8c58ffe79df63d23332376ce481cdc4753cc567 Mon Sep 17 00:00:00 2001 > From: Christoph Hellwig <hch@xxxxxx> > Date: Fri, 29 Jun 2018 10:54:10 +0200 > Subject: iomap: refactor iomap_dio_actor > > Split the function up into two helpers for the bio based I/O and hole > case, and a small helper to call the two. This separates the code a > little better in preparation for supporting I/O to inline data. > > Signed-off-by: Christoph Hellwig <hch@xxxxxx> Reviewed-by: Andreas Gruenbacher <agruenba@xxxxxxxxxx> > --- > fs/iomap.c | 88 ++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 52 insertions(+), 36 deletions(-) > > diff --git a/fs/iomap.c b/fs/iomap.c > index 7d1e9f45f098..f05c83773cbf 100644 > --- a/fs/iomap.c > +++ b/fs/iomap.c > @@ -963,10 +963,9 @@ iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos, > } > > static loff_t > -iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, > - void *data, struct iomap *iomap) > +iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > + struct iomap_dio *dio, struct iomap *iomap) > { > - struct iomap_dio *dio = data; > unsigned int blkbits = blksize_bits(bdev_logical_block_size(iomap->bdev)); > unsigned int fs_block_size = i_blocksize(inode), pad; > unsigned int align = iov_iter_alignment(dio->submit.iter); > @@ -980,41 +979,27 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, > if ((pos | length | align) & ((1 << blkbits) - 1)) > return -EINVAL; > > - switch (iomap->type) { > - case IOMAP_HOLE: > - if (WARN_ON_ONCE(dio->flags & IOMAP_DIO_WRITE)) > - return -EIO; > - /*FALLTHRU*/ > - case IOMAP_UNWRITTEN: > - if (!(dio->flags & IOMAP_DIO_WRITE)) { > - length = iov_iter_zero(length, dio->submit.iter); > - dio->size += length; > - return length; > - } > + if (iomap->type == IOMAP_UNWRITTEN) { > dio->flags |= IOMAP_DIO_UNWRITTEN; > need_zeroout = true; > - break; > - case IOMAP_MAPPED: > - if (iomap->flags & IOMAP_F_SHARED) > - dio->flags |= IOMAP_DIO_COW; > - if (iomap->flags & IOMAP_F_NEW) { > - need_zeroout = true; > - } else { > - /* > - * Use a FUA write if we need datasync semantics, this > - * is a pure data IO that doesn't require any metadata > - * updates and the underlying device supports FUA. This > - * allows us to avoid cache flushes on IO completion. > - */ > - if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) && > - (dio->flags & IOMAP_DIO_WRITE_FUA) && > - blk_queue_fua(bdev_get_queue(iomap->bdev))) > - use_fua = true; > - } > - break; > - default: > - WARN_ON_ONCE(1); > - return -EIO; > + } > + > + if (iomap->flags & IOMAP_F_SHARED) > + dio->flags |= IOMAP_DIO_COW; > + > + if (iomap->flags & IOMAP_F_NEW) { > + need_zeroout = true; > + } else { > + /* > + * Use a FUA write if we need datasync semantics, this > + * is a pure data IO that doesn't require any metadata > + * updates and the underlying device supports FUA. This > + * allows us to avoid cache flushes on IO completion. > + */ > + if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) && > + (dio->flags & IOMAP_DIO_WRITE_FUA) && > + blk_queue_fua(bdev_get_queue(iomap->bdev))) > + use_fua = true; > } > > /* > @@ -1093,6 +1078,37 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, > return copied; > } > > +static loff_t > +iomap_dio_hole_actor(loff_t length, struct iomap_dio *dio) > +{ > + length = iov_iter_zero(length, dio->submit.iter); > + dio->size += length; > + return length; > +} Just a minor nit: iomap_dio_hole_actor should come before iomap_dio_bio_actor. > + > +static loff_t > +iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, > + void *data, struct iomap *iomap) > +{ > + struct iomap_dio *dio = data; > + > + switch (iomap->type) { > + case IOMAP_HOLE: > + if (WARN_ON_ONCE(dio->flags & IOMAP_DIO_WRITE)) > + return -EIO; > + return iomap_dio_hole_actor(length, dio); > + case IOMAP_UNWRITTEN: > + if (!(dio->flags & IOMAP_DIO_WRITE)) > + return iomap_dio_hole_actor(length, dio); > + return iomap_dio_bio_actor(inode, pos, length, dio, iomap); > + case IOMAP_MAPPED: > + return iomap_dio_bio_actor(inode, pos, length, dio, iomap); > + default: > + WARN_ON_ONCE(1); > + return -EIO; > + } > +} > + > /* > * iomap_dio_rw() always completes O_[D]SYNC writes regardless of whether the IO > * is being issued as AIO or not. This allows us to optimise pure data writes > -- > 2.17.1 > Thanks, Andreas