From: Zhang Yi <[email protected]>
On a realtime inode, __xfs_bunmapi() could convert the unaligned extra
blocks to unwritten state, but it couldn't work as expected on truncate
down since the reserved block is zero in xfs_setattr_size(), fix this by
reserved XFS_IS_REALTIME_INODE blocks for realtime inode.
Signed-off-by: Zhang Yi <[email protected]>
---
fs/xfs/xfs_iops.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
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;
--
2.39.2
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: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
> - 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 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.
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.