On 1/21/20 9:29 PM, Allison Collins wrote:
On 1/21/20 4:15 PM, Darrick J. Wong wrote:
On Sat, Jan 18, 2020 at 03:50:30PM -0700, Allison Collins wrote:
Delayed operations cannot return error codes. So we must check for
these conditions first before starting set or remove operations
Answering my own question from earlier -- I see here you actually /are/
checking the attr existence w.r.t. ATTR_{CREATE,REPLACE} right after we
allocate a transaction and ILOCK the inode, so
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Alrighty, thank you!
Though I am wondering if you could discard the predicates from the
second patch in favor of doing a normal lookup of the attr with a zero
valuelen to determine if there's already an attribute?
I think I likely answered this in the response to that patch. Because
it's used as part of the remove procedures, we still need it. We could
make a simpler version just for this application I suppose, but it seems
like it'd just be extra code since we still need the former.
Thank you for the reviews!
Allison
--D
Signed-off-by: Allison Collins <allison.henderson@xxxxxxxxxx>
---
fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index a2673fe..e9d22c1 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -457,6 +457,14 @@ xfs_attr_set(
goto out_trans_cancel;
xfs_trans_ijoin(args.trans, dp, 0);
+
+ error = xfs_has_attr(&args);
+ if (error == -EEXIST && (name->type & ATTR_CREATE))
+ goto out_trans_cancel;
+
+ if (error == -ENOATTR && (name->type & ATTR_REPLACE))
+ goto out_trans_cancel;
+
error = xfs_attr_set_args(&args);
So I was thinking of adding this one to a smaller 3 patch series I
mentioned earlier. I was also thinking of adding in some asserts here:
ASSERT(error != -EEXIST)
ASSERT(error != -ENOATTR)
Just to make sure the changes are enforcing the behavioral changes that
we want. I thought this might be a good stabilizer to the rest of the
delayed attr series. Because chasing this bug back up through the log
replay is a much bigger PITA than catching it here. Thoughts?
if (error)
goto out_trans_cancel;
@@ -545,6 +553,10 @@ xfs_attr_remove(
*/
xfs_trans_ijoin(args.trans, dp, 0);
+ error = xfs_has_attr(&args);
+ if (error != -EEXIST)
+ goto out;
+
Here too:
ASSERT(error != -EEXIST)
Let me know what folks think. Thanks!
Allison
error = xfs_attr_remove_args(&args);
if (error)
goto out;
--
2.7.4
