Message ID | 20240311122255.2637311-2-yi.zhang@huaweicloud.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | xfs/iomap: fix non-atomic clone operation and don't update size when zeroing range post eof | expand |
On Mon, Mar 11, 2024 at 08:22:52PM +0800, Zhang Yi wrote: > From: Zhang Yi <yi.zhang@huawei.com> > > Commit 1aa91d9c9933 ("xfs: Add async buffered write support") replace > xfs_ilock(XFS_ILOCK_EXCL) with xfs_ilock_for_iomap() when locking the > writing inode, and a new variable lockmode is used to indicate the lock > mode. Although the lockmode should always be XFS_ILOCK_EXCL, it's still > better to use this variable instead of useing XFS_ILOCK_EXCL directly > when unlocking the inode. > > Fixes: 1aa91d9c9933 ("xfs: Add async buffered write support") AFAICT, xfs_ilock_for_iomap can change lockmode from SHARED->EXCL, but never changed away from EXCL, right? And xfs_buffered_write_iomap_begin sets it to EXCL (and never changes it), right? This seems like more of a code cleanup/logic bomb removal than an actual defect that someone could actually hit, correct? If the answers are {yes, yes, yes} then: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Signed-off-by: Zhang Yi <yi.zhang@huawei.com> > --- > fs/xfs/xfs_iomap.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c > index 18c8f168b153..ccf83e72d8ca 100644 > --- a/fs/xfs/xfs_iomap.c > +++ b/fs/xfs/xfs_iomap.c > @@ -1149,13 +1149,13 @@ xfs_buffered_write_iomap_begin( > * them out if the write happens to fail. > */ > seq = xfs_iomap_inode_sequence(ip, IOMAP_F_NEW); > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > + xfs_iunlock(ip, lockmode); > trace_xfs_iomap_alloc(ip, offset, count, allocfork, &imap); > return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, IOMAP_F_NEW, seq); > > found_imap: > seq = xfs_iomap_inode_sequence(ip, 0); > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > + xfs_iunlock(ip, lockmode); > return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, 0, seq); > > found_cow: > @@ -1165,17 +1165,17 @@ xfs_buffered_write_iomap_begin( > if (error) > goto out_unlock; > seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED); > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > + xfs_iunlock(ip, lockmode); > return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, > IOMAP_F_SHARED, seq); > } > > xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > + xfs_iunlock(ip, lockmode); > return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, 0, seq); > > out_unlock: > - xfs_iunlock(ip, XFS_ILOCK_EXCL); > + xfs_iunlock(ip, lockmode); > return error; > } > > -- > 2.39.2 > >
On 2024/3/11 23:34, Darrick J. Wong wrote: > On Mon, Mar 11, 2024 at 08:22:52PM +0800, Zhang Yi wrote: >> From: Zhang Yi <yi.zhang@huawei.com> >> >> Commit 1aa91d9c9933 ("xfs: Add async buffered write support") replace >> xfs_ilock(XFS_ILOCK_EXCL) with xfs_ilock_for_iomap() when locking the >> writing inode, and a new variable lockmode is used to indicate the lock >> mode. Although the lockmode should always be XFS_ILOCK_EXCL, it's still >> better to use this variable instead of useing XFS_ILOCK_EXCL directly >> when unlocking the inode. >> >> Fixes: 1aa91d9c9933 ("xfs: Add async buffered write support") > > AFAICT, xfs_ilock_for_iomap can change lockmode from SHARED->EXCL, but > never changed away from EXCL, right? Yes. > And xfs_buffered_write_iomap_begin > sets it to EXCL (and never changes it), right? Yes. > > This seems like more of a code cleanup/logic bomb removal than an actual > defect that someone could actually hit, correct? > Yes, it's not a real problem. > If the answers are {yes, yes, yes} then: > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > --D > > >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com> >> --- >> fs/xfs/xfs_iomap.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c >> index 18c8f168b153..ccf83e72d8ca 100644 >> --- a/fs/xfs/xfs_iomap.c >> +++ b/fs/xfs/xfs_iomap.c >> @@ -1149,13 +1149,13 @@ xfs_buffered_write_iomap_begin( >> * them out if the write happens to fail. >> */ >> seq = xfs_iomap_inode_sequence(ip, IOMAP_F_NEW); >> - xfs_iunlock(ip, XFS_ILOCK_EXCL); >> + xfs_iunlock(ip, lockmode); >> trace_xfs_iomap_alloc(ip, offset, count, allocfork, &imap); >> return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, IOMAP_F_NEW, seq); >> >> found_imap: >> seq = xfs_iomap_inode_sequence(ip, 0); >> - xfs_iunlock(ip, XFS_ILOCK_EXCL); >> + xfs_iunlock(ip, lockmode); >> return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, 0, seq); >> >> found_cow: >> @@ -1165,17 +1165,17 @@ xfs_buffered_write_iomap_begin( >> if (error) >> goto out_unlock; >> seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED); >> - xfs_iunlock(ip, XFS_ILOCK_EXCL); >> + xfs_iunlock(ip, lockmode); >> return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, >> IOMAP_F_SHARED, seq); >> } >> >> xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); >> - xfs_iunlock(ip, XFS_ILOCK_EXCL); >> + xfs_iunlock(ip, lockmode); >> return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, 0, seq); >> >> out_unlock: >> - xfs_iunlock(ip, XFS_ILOCK_EXCL); >> + xfs_iunlock(ip, lockmode); >> return error; >> } >> >> -- >> 2.39.2 >> >>
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c index 18c8f168b153..ccf83e72d8ca 100644 --- a/fs/xfs/xfs_iomap.c +++ b/fs/xfs/xfs_iomap.c @@ -1149,13 +1149,13 @@ xfs_buffered_write_iomap_begin( * them out if the write happens to fail. */ seq = xfs_iomap_inode_sequence(ip, IOMAP_F_NEW); - xfs_iunlock(ip, XFS_ILOCK_EXCL); + xfs_iunlock(ip, lockmode); trace_xfs_iomap_alloc(ip, offset, count, allocfork, &imap); return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, IOMAP_F_NEW, seq); found_imap: seq = xfs_iomap_inode_sequence(ip, 0); - xfs_iunlock(ip, XFS_ILOCK_EXCL); + xfs_iunlock(ip, lockmode); return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, 0, seq); found_cow: @@ -1165,17 +1165,17 @@ xfs_buffered_write_iomap_begin( if (error) goto out_unlock; seq = xfs_iomap_inode_sequence(ip, IOMAP_F_SHARED); - xfs_iunlock(ip, XFS_ILOCK_EXCL); + xfs_iunlock(ip, lockmode); return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, IOMAP_F_SHARED, seq); } xfs_trim_extent(&cmap, offset_fsb, imap.br_startoff - offset_fsb); - xfs_iunlock(ip, XFS_ILOCK_EXCL); + xfs_iunlock(ip, lockmode); return xfs_bmbt_to_iomap(ip, iomap, &cmap, flags, 0, seq); out_unlock: - xfs_iunlock(ip, XFS_ILOCK_EXCL); + xfs_iunlock(ip, lockmode); return error; }