| Message ID | 20200625233018.14585-15-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Thu, Jun 25, 2020 at 04:30:07PM -0700, Allison Collins wrote: > The xfs_trans_roll in _removename is not needed because invalidating > blocks is an incore-only change. This is analogous to the non-remote > remove case where an entry is removed and a potential dabtree join > occurs under the same transaction. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- Ok, but I think we should be a bit more descriptive in the commit log so the reasoning is available for historical reference. For example: "A transaction roll is not necessary immediately after setting the INCOMPLETE flag when removing a node xattr entry with remote value blocks. The remote block invalidation that immediately follows setting the flag is an in-core only change. The next step after that is to start unmapping the remote blocks from the attr fork, but the xattr remove transaction reservation includes reservation for full tree splits of the dabtree and bmap tree. The remote block unmap code will roll the transaction as extents are unmapped and freed." With something like that in place: Reviewed-by: Brian Foster <bfoster@redhat.com> > fs/xfs/libxfs/xfs_attr.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 1a78023..f1becca 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -1148,10 +1148,6 @@ xfs_attr_node_removename( > if (error) > goto out; > > - error = xfs_trans_roll_inode(&args->trans, args->dp); > - if (error) > - goto out; > - > error = xfs_attr_rmtval_invalidate(args); > if (error) > return error; > -- > 2.7.4 >
On 7/8/20 5:42 AM, Brian Foster wrote: > On Thu, Jun 25, 2020 at 04:30:07PM -0700, Allison Collins wrote: >> The xfs_trans_roll in _removename is not needed because invalidating >> blocks is an incore-only change. This is analogous to the non-remote >> remove case where an entry is removed and a potential dabtree join >> occurs under the same transaction. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- > > Ok, but I think we should be a bit more descriptive in the commit log so > the reasoning is available for historical reference. For example: > > "A transaction roll is not necessary immediately after setting the > INCOMPLETE flag when removing a node xattr entry with remote value > blocks. The remote block invalidation that immediately follows setting > the flag is an in-core only change. The next step after that is to start > unmapping the remote blocks from the attr fork, but the xattr remove > transaction reservation includes reservation for full tree splits of the > dabtree and bmap tree. The remote block unmap code will roll the > transaction as extents are unmapped and freed." Ok, that is a lot more detailed. > > With something like that in place: > > Reviewed-by: Brian Foster <bfoster@redhat.com> Sure, will update. Thanks! Allison > >> fs/xfs/libxfs/xfs_attr.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 1a78023..f1becca 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -1148,10 +1148,6 @@ xfs_attr_node_removename( >> if (error) >> goto out; >> >> - error = xfs_trans_roll_inode(&args->trans, args->dp); >> - if (error) >> - goto out; >> - >> error = xfs_attr_rmtval_invalidate(args); >> if (error) >> return error; >> -- >> 2.7.4 >> >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 1a78023..f1becca 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1148,10 +1148,6 @@ xfs_attr_node_removename( if (error) goto out; - error = xfs_trans_roll_inode(&args->trans, args->dp); - if (error) - goto out; - error = xfs_attr_rmtval_invalidate(args); if (error) return error;
The xfs_trans_roll in _removename is not needed because invalidating blocks is an incore-only change. This is analogous to the non-remote remove case where an entry is removed and a potential dabtree join occurs under the same transaction. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 4 ---- 1 file changed, 4 deletions(-)