| Message ID | 20220426174335.4004987-7-shr@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | io-uring/xfs: support async buffered writes | expand |
On Tue, Apr 26, 2022 at 10:43:23AM -0700, Stefan Roesch wrote: > This adds the async buffered write support to the iomap layer of XFS. If > a lock cannot be acquired or additional reads need to be performed, the > request will return -EAGAIN in case this is an async buffered write request. > > Signed-off-by: Stefan Roesch <shr@fb.com> > --- > fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index e552ce541ec2..80b6c48e88af 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin( > bool eof = false, cow_eof = false, shared = false; > int allocfork = XFS_DATA_FORK; > int error = 0; > + bool no_wait = (flags & IOMAP_NOWAIT); > > if (xfs_is_shutdown(mp)) > return -EIO; > > /* we can't use delayed allocations when using extent size hints */ > - if (xfs_get_extsz_hint(ip)) > + if (xfs_get_extsz_hint(ip)) { > + if (no_wait) > + return -EAGAIN; > + > return xfs_direct_write_iomap_begin(inode, offset, count, > flags, iomap, srcmap); Why can't we do IOMAP_NOWAIT allocation here? xfs_direct_write_iomap_begin() supports IOMAP_NOWAIT semantics just fine - that's what the direct IO path uses... > + } > > ASSERT(!XFS_IS_REALTIME_INODE(ip)); > > - xfs_ilock(ip, XFS_ILOCK_EXCL); > + if (no_wait) { > + if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) > + return -EAGAIN; > + } else { > + xfs_ilock(ip, XFS_ILOCK_EXCL); > + } handled by xfs_ilock_for_iomap(). > > if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(&ip->i_df)) || > XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) { > @@ -902,6 +912,11 @@ xfs_buffered_write_iomap_begin( > > XFS_STATS_INC(mp, xs_blk_mapw); > > + if (no_wait && xfs_need_iread_extents(XFS_IFORK_PTR(ip, XFS_DATA_FORK))) { > + error = -EAGAIN; > + goto out_unlock; > + } > + Also handled by xfs_ilock_for_iomap(). > error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK); > if (error) > goto out_unlock; > @@ -933,6 +948,10 @@ xfs_buffered_write_iomap_begin( > if (xfs_is_cow_inode(ip)) { > if (!ip->i_cowfp) { > ASSERT(!xfs_is_reflink_inode(ip)); > + if (no_wait) { > + error = -EAGAIN; > + goto out_unlock; > + } > xfs_ifork_init_cow(ip); > } Also handled by xfs_ilock_for_iomap(). > cow_eof = !xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, > @@ -956,6 +975,11 @@ xfs_buffered_write_iomap_begin( > goto found_imap; > } > > + if (no_wait) { > + error = -EAGAIN; > + goto out_unlock; > + } > + Won't get here because this is COW path, and xfs_ilock_for_iomap() handles returning -EAGAIN for that case. > xfs_trim_extent(&imap, offset_fsb, end_fsb - offset_fsb); > > /* Trim the mapping to the nearest shared extent boundary. */ > @@ -993,6 +1017,11 @@ xfs_buffered_write_iomap_begin( > allocfork = XFS_COW_FORK; > } > > + if (no_wait) { > + error = -EAGAIN; > + goto out_unlock; > + } Why? Delayed allocation doesn't block... Cheers, Dave.
On 4/26/22 3:54 PM, Dave Chinner wrote: > On Tue, Apr 26, 2022 at 10:43:23AM -0700, Stefan Roesch wrote: >> This adds the async buffered write support to the iomap layer of XFS. If >> a lock cannot be acquired or additional reads need to be performed, the >> request will return -EAGAIN in case this is an async buffered write request. >> >> Signed-off-by: Stefan Roesch <shr@fb.com> >> --- >> fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++-- >> 1 file changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c >> index e552ce541ec2..80b6c48e88af 100644 >> --- a/fs/xfs/xfs_iomap.c >> +++ b/fs/xfs/xfs_iomap.c >> @@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin( >> bool eof = false, cow_eof = false, shared = false; >> int allocfork = XFS_DATA_FORK; >> int error = 0; >> + bool no_wait = (flags & IOMAP_NOWAIT); >> >> if (xfs_is_shutdown(mp)) >> return -EIO; >> >> /* we can't use delayed allocations when using extent size hints */ >> - if (xfs_get_extsz_hint(ip)) >> + if (xfs_get_extsz_hint(ip)) { >> + if (no_wait) >> + return -EAGAIN; >> + >> return xfs_direct_write_iomap_begin(inode, offset, count, >> flags, iomap, srcmap); > > Why can't we do IOMAP_NOWAIT allocation here? > xfs_direct_write_iomap_begin() supports IOMAP_NOWAIT semantics just > fine - that's what the direct IO path uses... I'll make that change in the next version. > >> + } >> >> ASSERT(!XFS_IS_REALTIME_INODE(ip)); >> >> - xfs_ilock(ip, XFS_ILOCK_EXCL); >> + if (no_wait) { >> + if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) >> + return -EAGAIN; >> + } else { >> + xfs_ilock(ip, XFS_ILOCK_EXCL); >> + } > > handled by xfs_ilock_for_iomap(). The helper xfs_ilock_for_iomap cannot be used for this purpose. The function xfs_ilock_for_iomap tries to deduce the lock mode. For the function xfs_file_buffered_write() the lock mode must be exclusive. However this is not always the case when the helper xfs_ilock_for_iomap is used. There are cases where xfs_is_cow_inode(() is not true. I'll add a new helper that will be used here. >> >> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(&ip->i_df)) || >> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) { >> @@ -902,6 +912,11 @@ xfs_buffered_write_iomap_begin( >> >> XFS_STATS_INC(mp, xs_blk_mapw); >> >> + if (no_wait && xfs_need_iread_extents(XFS_IFORK_PTR(ip, XFS_DATA_FORK))) { >> + error = -EAGAIN; >> + goto out_unlock; >> + } >> + > > Also handled by xfs_ilock_for_iomap(). I'll remove this check with the next version. > >> error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK); >> if (error) >> goto out_unlock; >> @@ -933,6 +948,10 @@ xfs_buffered_write_iomap_begin( >> if (xfs_is_cow_inode(ip)) { >> if (!ip->i_cowfp) { >> ASSERT(!xfs_is_reflink_inode(ip)); >> + if (no_wait) { >> + error = -EAGAIN; >> + goto out_unlock; >> + } >> xfs_ifork_init_cow(ip); >> } > > Also handled by xfs_ilock_for_iomap(). > I'll remove this check with the next version. >> cow_eof = !xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, >> @@ -956,6 +975,11 @@ xfs_buffered_write_iomap_begin( >> goto found_imap; >> } >> >> + if (no_wait) { >> + error = -EAGAIN; >> + goto out_unlock; >> + } >> + > > Won't get here because this is COW path, and xfs_ilock_for_iomap() > handles returning -EAGAIN for that case. I'll remove this check with the next version. > >> xfs_trim_extent(&imap, offset_fsb, end_fsb - offset_fsb); >> >> /* Trim the mapping to the nearest shared extent boundary. */ >> @@ -993,6 +1017,11 @@ xfs_buffered_write_iomap_begin( >> allocfork = XFS_COW_FORK; >> } >> >> + if (no_wait) { >> + error = -EAGAIN; >> + goto out_unlock; >> + } > > Why? Delayed allocation doesn't block... I'll remove this check with the next version. > > Cheers, > > Dave. >
On Thu, Apr 28, 2022 at 01:03:49PM -0700, Stefan Roesch wrote: > > > On 4/26/22 3:54 PM, Dave Chinner wrote: > > On Tue, Apr 26, 2022 at 10:43:23AM -0700, Stefan Roesch wrote: > >> This adds the async buffered write support to the iomap layer of XFS. If > >> a lock cannot be acquired or additional reads need to be performed, the > >> request will return -EAGAIN in case this is an async buffered write request. > >> > >> Signed-off-by: Stefan Roesch <shr@fb.com> > >> --- > >> fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++-- > >> 1 file changed, 31 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > >> index e552ce541ec2..80b6c48e88af 100644 > >> --- a/fs/xfs/xfs_iomap.c > >> +++ b/fs/xfs/xfs_iomap.c > >> @@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin( > >> bool eof = false, cow_eof = false, shared = false; > >> int allocfork = XFS_DATA_FORK; > >> int error = 0; > >> + bool no_wait = (flags & IOMAP_NOWAIT); > >> > >> if (xfs_is_shutdown(mp)) > >> return -EIO; > >> > >> /* we can't use delayed allocations when using extent size hints */ > >> - if (xfs_get_extsz_hint(ip)) > >> + if (xfs_get_extsz_hint(ip)) { > >> + if (no_wait) > >> + return -EAGAIN; > >> + > >> return xfs_direct_write_iomap_begin(inode, offset, count, > >> flags, iomap, srcmap); > > > > Why can't we do IOMAP_NOWAIT allocation here? > > xfs_direct_write_iomap_begin() supports IOMAP_NOWAIT semantics just > > fine - that's what the direct IO path uses... > > I'll make that change in the next version. > > > >> + } > >> > >> ASSERT(!XFS_IS_REALTIME_INODE(ip)); > >> > >> - xfs_ilock(ip, XFS_ILOCK_EXCL); > >> + if (no_wait) { > >> + if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) > >> + return -EAGAIN; > >> + } else { > >> + xfs_ilock(ip, XFS_ILOCK_EXCL); > >> + } > > > > handled by xfs_ilock_for_iomap(). > > The helper xfs_ilock_for_iomap cannot be used for this purpose. The function > xfs_ilock_for_iomap tries to deduce the lock mode. For the function xfs_file_buffered_write() > the lock mode must be exclusive. However this is not always the case when the > helper xfs_ilock_for_iomap is used. There are cases where xfs_is_cow_inode(() is not > true. That's trivially fixed by having the caller initialise the lock_mode to what they want, rather than have the function set the initial type to SHARED.... > I'll add a new helper that will be used here. ... so I don't think a new helper is needed. --Dave.
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index e552ce541ec2..80b6c48e88af 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -881,18 +881,28 @@ xfs_buffered_write_iomap_begin( bool eof = false, cow_eof = false, shared = false; int allocfork = XFS_DATA_FORK; int error = 0; + bool no_wait = (flags & IOMAP_NOWAIT); if (xfs_is_shutdown(mp)) return -EIO; /* we can't use delayed allocations when using extent size hints */ - if (xfs_get_extsz_hint(ip)) + if (xfs_get_extsz_hint(ip)) { + if (no_wait) + return -EAGAIN; + return xfs_direct_write_iomap_begin(inode, offset, count, flags, iomap, srcmap); + } ASSERT(!XFS_IS_REALTIME_INODE(ip)); - xfs_ilock(ip, XFS_ILOCK_EXCL); + if (no_wait) { + if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) + return -EAGAIN; + } else { + xfs_ilock(ip, XFS_ILOCK_EXCL); + } if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(&ip->i_df)) || XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) { @@ -902,6 +912,11 @@ xfs_buffered_write_iomap_begin( XFS_STATS_INC(mp, xs_blk_mapw); + if (no_wait && xfs_need_iread_extents(XFS_IFORK_PTR(ip, XFS_DATA_FORK))) { + error = -EAGAIN; + goto out_unlock; + } + error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK); if (error) goto out_unlock; @@ -933,6 +948,10 @@ xfs_buffered_write_iomap_begin( if (xfs_is_cow_inode(ip)) { if (!ip->i_cowfp) { ASSERT(!xfs_is_reflink_inode(ip)); + if (no_wait) { + error = -EAGAIN; + goto out_unlock; + } xfs_ifork_init_cow(ip); } cow_eof = !xfs_iext_lookup_extent(ip, ip->i_cowfp, offset_fsb, @@ -956,6 +975,11 @@ xfs_buffered_write_iomap_begin( goto found_imap; } + if (no_wait) { + error = -EAGAIN; + goto out_unlock; + } + xfs_trim_extent(&imap, offset_fsb, end_fsb - offset_fsb); /* Trim the mapping to the nearest shared extent boundary. */ @@ -993,6 +1017,11 @@ xfs_buffered_write_iomap_begin( allocfork = XFS_COW_FORK; } + if (no_wait) { + error = -EAGAIN; + goto out_unlock; + } + error = xfs_qm_dqattach_locked(ip, false); if (error) goto out_unlock;
This adds the async buffered write support to the iomap layer of XFS. If a lock cannot be acquired or additional reads need to be performed, the request will return -EAGAIN in case this is an async buffered write request. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/xfs/xfs_iomap.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)