| Message ID | 20240529095206.2568162-8-yi.zhang@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iomap/xfs: fix stale data exposure when truncating realtime inodes | expand |
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); > + resblks = XFS_IS_REALTIME_INODE(ip) ? XFS_DIOSTRAT_SPACE_RES(mp, 0) : 0; This probably wants a comment explaining that we need the block reservation for bmap btree block allocations / splits that can happen because we can split a written extent into one written and one unwritten, while for the data fork we'll always just shorten or remove extents. I'd also find this more readable if resblks was initialized to 0, and this became a: if (XFS_IS_REALTIME_INODE(ip)) resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
On Fri, May 31, 2024 at 05:42:37AM -0700, Christoph Hellwig wrote: > > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); > > + resblks = XFS_IS_REALTIME_INODE(ip) ? XFS_DIOSTRAT_SPACE_RES(mp, 0) : 0; > > This probably wants a comment explaining that we need the block > reservation for bmap btree block allocations / splits that can happen > because we can split a written extent into one written and one > unwritten, while for the data fork we'll always just shorten or > remove extents. "for the data fork"? <confused> This always runs on the data fork. Did you mean "for files with alloc unit > 1 fsblock"? > I'd also find this more readable if resblks was initialized to 0, > and this became a: > > if (XFS_IS_REALTIME_INODE(ip)) > resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0); Agreed. --D
On Fri, May 31, 2024 at 07:10:00AM -0700, Darrick J. Wong wrote: > On Fri, May 31, 2024 at 05:42:37AM -0700, Christoph Hellwig wrote: > > > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); > > > + resblks = XFS_IS_REALTIME_INODE(ip) ? XFS_DIOSTRAT_SPACE_RES(mp, 0) : 0; > > > > This probably wants a comment explaining that we need the block > > reservation for bmap btree block allocations / splits that can happen > > because we can split a written extent into one written and one > > unwritten, while for the data fork we'll always just shorten or > > remove extents. > > "for the data fork"? <confused> > > This always runs on the data fork. Did you mean "for files with alloc > unit > 1 fsblock"? Sorry, it was meant to say for the data device. My whole journey to check if this could get called for the attr fork twisted my mind. But you have a good point that even for the rt device we only need the reservation for an rtextsize > 1.
On Fri, May 31, 2024 at 07:13:05AM -0700, Christoph Hellwig wrote: > On Fri, May 31, 2024 at 07:10:00AM -0700, Darrick J. Wong wrote: > > On Fri, May 31, 2024 at 05:42:37AM -0700, Christoph Hellwig wrote: > > > > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); > > > > + resblks = XFS_IS_REALTIME_INODE(ip) ? XFS_DIOSTRAT_SPACE_RES(mp, 0) : 0; > > > > > > This probably wants a comment explaining that we need the block > > > reservation for bmap btree block allocations / splits that can happen > > > because we can split a written extent into one written and one > > > unwritten, while for the data fork we'll always just shorten or > > > remove extents. > > > > "for the data fork"? <confused> > > > > This always runs on the data fork. Did you mean "for files with alloc > > unit > 1 fsblock"? > > Sorry, it was meant to say for the data device. My whole journey > to check if this could get called for the attr fork twisted my mind. I really hope not -- all writes to the attr fork have known sizes at syscall time, and appending doesn't even make sense. > But you have a good point that even for the rt device we only need > the reservation for an rtextsize > 1. <nod> --D
On Fri, May 31, 2024 at 08:29:22AM -0700, Darrick J. Wong wrote: > > > > Sorry, it was meant to say for the data device. My whole journey > > to check if this could get called for the attr fork twisted my mind. > > I really hope not -- all writes to the attr fork have known sizes at > syscall time, and appending doesn't even make sense. It's obviously not. I just had to go out and actually read the code before commenting.
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c index ec7b7bdf8825..c53de5e6ef66 100644 --- a/fs/xfs/xfs_iops.c +++ b/fs/xfs/xfs_iops.c @@ -17,6 +17,8 @@ #include "xfs_da_btree.h" #include "xfs_attr.h" #include "xfs_trans.h" +#include "xfs_trans_space.h" +#include "xfs_bmap_btree.h" #include "xfs_trace.h" #include "xfs_icache.h" #include "xfs_symlink.h" @@ -811,6 +813,7 @@ xfs_setattr_size( struct xfs_trans *tp; int error; uint lock_flags = 0; + uint resblks; bool did_zeroing = false; bool write_back = false; @@ -932,7 +935,9 @@ xfs_setattr_size( } } - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, 0, 0, 0, &tp); + resblks = XFS_IS_REALTIME_INODE(ip) ? XFS_DIOSTRAT_SPACE_RES(mp, 0) : 0; + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_itruncate, resblks, + 0, 0, &tp); if (error) return error;