| Message ID | 316306.1716306586@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | netfs: Fix setting of BDP_ASYNC from iocb flags | expand |
On 5/21/24 9:49 AM, David Howells wrote: > Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather > than if IOCB_SYNC is not set. It reflects asynchronicity in the sense of > not waiting rather than synchronicity in the sense of not returning until > the op is complete. > > Without this, generic/590 fails on cifs in strict caching mode with a > complaint that one of the writes fails with EAGAIN. The test can be > distilled down to: > > mount -t cifs /my/share /mnt -ostuff > xfs_io -i -c 'falloc 0 8191M -c fsync -f /mnt/file > xfs_io -i -c 'pwrite -b 1M -W 0 8191M' /mnt/file Looks good to me: Reviewed-by: Jens Axboe <axboe@kernel.dk> However, I'll note that BDP_ASYNC is horribly named, it should be BDP_NOWAIT instead. But that's a separate thing, fix looks correct as-is.
Jens Axboe <axboe@kernel.dk> wrote: > However, I'll note that BDP_ASYNC is horribly named, it should be > BDP_NOWAIT instead. But that's a separate thing, fix looks correct > as-is. I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the code. David
On 5/21/24 9:54 AM, David Howells wrote: > Jens Axboe <axboe@kernel.dk> wrote: > >> However, I'll note that BDP_ASYNC is horribly named, it should be >> BDP_NOWAIT instead. But that's a separate thing, fix looks correct >> as-is. > > I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the > code. It is, something submitted with RWF_NOWAIT should have IOCB_NOWAIT set. But RWF_NOWAIT isn't the sole user of IOCB_NOWAIT, and no assumptions should be made about whether something is sync or async based on whether or not RWF_NOWAIT is set. Those aren't related other than _some_ proper async IO will have IOCB_NOWAIT set, and others will not.
Jens Axboe <axboe@kernel.dk> wrote: > On 5/21/24 9:54 AM, David Howells wrote: > > Jens Axboe <axboe@kernel.dk> wrote: > > > >> However, I'll note that BDP_ASYNC is horribly named, it should be > >> BDP_NOWAIT instead. But that's a separate thing, fix looks correct > >> as-is. > > > > I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the > > code. > > It is, something submitted with RWF_NOWAIT should have IOCB_NOWAIT set. > But RWF_NOWAIT isn't the sole user of IOCB_NOWAIT, and no assumptions > should be made about whether something is sync or async based on whether > or not RWF_NOWAIT is set. Those aren't related other than _some_ proper > async IO will have IOCB_NOWAIT set, and others will not. Are you sure? RWF_NOWAIT seems to set IOCB_NOIO. David
On 5/21/24 2:05 PM, David Howells wrote: > Jens Axboe <axboe@kernel.dk> wrote: > >> On 5/21/24 9:54 AM, David Howells wrote: >>> Jens Axboe <axboe@kernel.dk> wrote: >>> >>>> However, I'll note that BDP_ASYNC is horribly named, it should be >>>> BDP_NOWAIT instead. But that's a separate thing, fix looks correct >>>> as-is. >>> >>> I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the >>> code. >> >> It is, something submitted with RWF_NOWAIT should have IOCB_NOWAIT set. >> But RWF_NOWAIT isn't the sole user of IOCB_NOWAIT, and no assumptions >> should be made about whether something is sync or async based on whether >> or not RWF_NOWAIT is set. Those aren't related other than _some_ proper >> async IO will have IOCB_NOWAIT set, and others will not. > > Are you sure? RWF_NOWAIT seems to set IOCB_NOIO. As it should, no-wait should imply not blocking on other IO. This is completely orthogonal to whether or not it's async or sync IO. I have a distinct feeling we're talking past each other :-)
On Tue, 21 May 2024 16:49:46 +0100, David Howells wrote: > Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather > than if IOCB_SYNC is not set. It reflects asynchronicity in the sense of > not waiting rather than synchronicity in the sense of not returning until > the op is complete. > > Without this, generic/590 fails on cifs in strict caching mode with a > complaint that one of the writes fails with EAGAIN. The test can be > distilled down to: > > [...] Applied to the vfs.fixes branch of the vfs/vfs.git tree. Patches in the vfs.fixes branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.fixes [1/1] netfs: Fix setting of BDP_ASYNC from iocb flags https://git.kernel.org/vfs/vfs/c/33c9d7477ef1
You can add my Tested-by if you would like On Wed, May 22, 2024 at 2:14 AM Christian Brauner <brauner@kernel.org> wrote: > > On Tue, 21 May 2024 16:49:46 +0100, David Howells wrote: > > Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather > > than if IOCB_SYNC is not set. It reflects asynchronicity in the sense of > > not waiting rather than synchronicity in the sense of not returning until > > the op is complete. > > > > Without this, generic/590 fails on cifs in strict caching mode with a > > complaint that one of the writes fails with EAGAIN. The test can be > > distilled down to: > > > > [...] > > Applied to the vfs.fixes branch of the vfs/vfs.git tree. > Patches in the vfs.fixes branch should appear in linux-next soon. > > Please report any outstanding bugs that were missed during review in a > new review to the original patch series allowing us to drop it. > > It's encouraged to provide Acked-bys and Reviewed-bys even though the > patch has now been applied. If possible patch trailers will be updated. > > Note that commit hashes shown below are subject to change due to rebase, > trailer updates or similar. If in doubt, please check the listed branch. > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git > branch: vfs.fixes > > [1/1] netfs: Fix setting of BDP_ASYNC from iocb flags > https://git.kernel.org/vfs/vfs/c/33c9d7477ef1 >
diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c index 1121601536d1..07bc1fd43530 100644 --- a/fs/netfs/buffered_write.c +++ b/fs/netfs/buffered_write.c @@ -181,7 +181,7 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter, struct folio *folio, *writethrough = NULL; enum netfs_how_to_modify howto; enum netfs_folio_trace trace; - unsigned int bdp_flags = (iocb->ki_flags & IOCB_SYNC) ? 0: BDP_ASYNC; + unsigned int bdp_flags = (iocb->ki_flags & IOCB_NOWAIT) ? BDP_ASYNC : 0; ssize_t written = 0, ret, ret2; loff_t i_size, pos = iocb->ki_pos, from, to; size_t max_chunk = PAGE_SIZE << MAX_PAGECACHE_ORDER;
Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather than if IOCB_SYNC is not set. It reflects asynchronicity in the sense of not waiting rather than synchronicity in the sense of not returning until the op is complete. Without this, generic/590 fails on cifs in strict caching mode with a complaint that one of the writes fails with EAGAIN. The test can be distilled down to: mount -t cifs /my/share /mnt -ostuff xfs_io -i -c 'falloc 0 8191M -c fsync -f /mnt/file xfs_io -i -c 'pwrite -b 1M -W 0 8191M' /mnt/file Fixes: c38f4e96e605 ("netfs: Provide func to copy data to pagecache for buffered write") Signed-off-by: David Howells <dhowells@redhat.com> cc: Jeff Layton <jlayton@kernel.org> cc: Enzo Matsumiya <ematsumiya@suse.de> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: netfs@lists.linux.dev cc: v9fs@lists.linux.dev cc: linux-afs@lists.infradead.org cc: linux-cifs@vger.kernel.org cc: linux-fsdevel@vger.kernel.org --- fs/netfs/buffered_write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)