| Message ID | x49r1z86e1d.fsf@segfault.boston.devel.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dax: pass NOWAIT flag to iomap_apply | expand |
On Wed, Feb 05, 2020 at 02:15:58PM -0500, Jeff Moyer wrote: > fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o > dax". The reason is that the initial pwrite to an empty file with the > RWF_NOWAIT flag set does not return -EAGAIN. It turns out that > dax_iomap_rw doesn't pass that flag through to iomap_apply. > > With this patch applied, generic/471 passes for me. > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Feb 5, 2020 at 11:28 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Wed, Feb 05, 2020 at 02:15:58PM -0500, Jeff Moyer wrote: > > fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o > > dax". The reason is that the initial pwrite to an empty file with the > > RWF_NOWAIT flag set does not return -EAGAIN. It turns out that > > dax_iomap_rw doesn't pass that flag through to iomap_apply. > > > > With this patch applied, generic/471 passes for me. > > > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > > Looks good, > > Reviewed-by: Christoph Hellwig <hch@lst.de> Applied.
On Wed 05-02-20 14:15:58, Jeff Moyer wrote: > fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o > dax". The reason is that the initial pwrite to an empty file with the > RWF_NOWAIT flag set does not return -EAGAIN. It turns out that > dax_iomap_rw doesn't pass that flag through to iomap_apply. > > With this patch applied, generic/471 passes for me. > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> The patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> BTW, I've just noticed ext4 seems to be buggy in this regard and even this patch doesn't fix it. So I guess you've been using XFS for testing this? Honza > diff --git a/fs/dax.c b/fs/dax.c > index 1f1f0201cad1..0b0d8819cb1b 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -1207,6 +1207,9 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter, > lockdep_assert_held(&inode->i_rwsem); > } > > + if (iocb->ki_flags & IOCB_NOWAIT) > + flags |= IOMAP_NOWAIT; > + > while (iov_iter_count(iter)) { > ret = iomap_apply(inode, pos, iov_iter_count(iter), flags, ops, > iter, dax_iomap_actor); >
Jan Kara <jack@suse.cz> writes: > On Wed 05-02-20 14:15:58, Jeff Moyer wrote: >> fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o >> dax". The reason is that the initial pwrite to an empty file with the >> RWF_NOWAIT flag set does not return -EAGAIN. It turns out that >> dax_iomap_rw doesn't pass that flag through to iomap_apply. >> >> With this patch applied, generic/471 passes for me. >> >> Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > > The patch looks good to me. You can add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > BTW, I've just noticed ext4 seems to be buggy in this regard and even this > patch doesn't fix it. So I guess you've been using XFS for testing this? That's right, sorry I didn't mention that. Will you send a patch for ext4, or do you want me to look into it? Thanks! Jeff
On Thu 06-02-20 09:33:39, Jeff Moyer wrote: > Jan Kara <jack@suse.cz> writes: > > > On Wed 05-02-20 14:15:58, Jeff Moyer wrote: > >> fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o > >> dax". The reason is that the initial pwrite to an empty file with the > >> RWF_NOWAIT flag set does not return -EAGAIN. It turns out that > >> dax_iomap_rw doesn't pass that flag through to iomap_apply. > >> > >> With this patch applied, generic/471 passes for me. > >> > >> Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > > > > The patch looks good to me. You can add: > > > > Reviewed-by: Jan Kara <jack@suse.cz> > > > > BTW, I've just noticed ext4 seems to be buggy in this regard and even this > > patch doesn't fix it. So I guess you've been using XFS for testing this? > > That's right, sorry I didn't mention that. Will you send a patch for > ext4, or do you want me to look into it? I've taken down a note in todo list to eventually look into that but if you can have a look, I'm more than happy to remove that entry :). Honza
Jan Kara <jack@suse.cz> writes: > On Thu 06-02-20 09:33:39, Jeff Moyer wrote: >> Jan Kara <jack@suse.cz> writes: >> >> > On Wed 05-02-20 14:15:58, Jeff Moyer wrote: >> >> fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o >> >> dax". The reason is that the initial pwrite to an empty file with the >> >> RWF_NOWAIT flag set does not return -EAGAIN. It turns out that >> >> dax_iomap_rw doesn't pass that flag through to iomap_apply. >> >> >> >> With this patch applied, generic/471 passes for me. >> >> >> >> Signed-off-by: Jeff Moyer <jmoyer@redhat.com> >> > >> > The patch looks good to me. You can add: >> > >> > Reviewed-by: Jan Kara <jack@suse.cz> >> > >> > BTW, I've just noticed ext4 seems to be buggy in this regard and even this >> > patch doesn't fix it. So I guess you've been using XFS for testing this? >> >> That's right, sorry I didn't mention that. Will you send a patch for >> ext4, or do you want me to look into it? > > I've taken down a note in todo list to eventually look into that but if you > can have a look, I'm more than happy to remove that entry :). OK, I'll take a look. -Jeff
diff --git a/fs/dax.c b/fs/dax.c index 1f1f0201cad1..0b0d8819cb1b 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1207,6 +1207,9 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter, lockdep_assert_held(&inode->i_rwsem); } + if (iocb->ki_flags & IOCB_NOWAIT) + flags |= IOMAP_NOWAIT; + while (iov_iter_count(iter)) { ret = iomap_apply(inode, pos, iov_iter_count(iter), flags, ops, iter, dax_iomap_actor);
fstests generic/471 reports a failure when run with MOUNT_OPTIONS="-o dax". The reason is that the initial pwrite to an empty file with the RWF_NOWAIT flag set does not return -EAGAIN. It turns out that dax_iomap_rw doesn't pass that flag through to iomap_apply. With this patch applied, generic/471 passes for me. Signed-off-by: Jeff Moyer <jmoyer@redhat.com>