| Message ID | 20180629085614.GB16042@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29 June 2018 at 10:56, Christoph Hellwig <hch@lst.de> 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@lst.de> > 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@lst.de> Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com> > --- > 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
On Fri, Jun 29, 2018 at 04:40:40PM +0200, Andreas Gruenbacher wrote: > On 29 June 2018 at 10:56, Christoph Hellwig <hch@lst.de> 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". Darrick now has a real iomap merge branch which replaced it. Before formally submitting the patch I'd also like to verify that it does not regress for XFS by doing a full xfstests run.
On 29 June 2018 at 18:01, Christoph Hellwig <hch@lst.de> wrote: > On Fri, Jun 29, 2018 at 04:40:40PM +0200, Andreas Gruenbacher wrote: >> On 29 June 2018 at 10:56, Christoph Hellwig <hch@lst.de> 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". > > Darrick now has a real iomap merge branch which replaced it. Where is it? Not in https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git it seems. > Before formally submitting the patch I'd also like to verify that it does > not regress for XFS by doing a full xfstests run. Thanks, Andreas
On Fri, Jun 29, 2018 at 07:02:07PM +0200, Andreas Gruenbacher wrote: > On 29 June 2018 at 18:01, Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Jun 29, 2018 at 04:40:40PM +0200, Andreas Gruenbacher wrote: > >> On 29 June 2018 at 10:56, Christoph Hellwig <hch@lst.de> 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". > > > > Darrick now has a real iomap merge branch which replaced it. > > Where is it? Not in > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git > it seems. https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/log/?h=iomap-4.19-merge
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; +} + +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