On 1/26/20 3:28 PM, Darrick J. Wong wrote:
On Sat, Jan 25, 2020 at 09:41:47AM -0700, Allison Collins wrote:
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?
Er, are the asserts to check that xfs_attr_set_args never returns
EEXIST/ENOATTR? I'm not sure why you'd have to chase this through log
replay?
Yes, the idea is that EEXIST and ENOATTR are supposed to be found and
returned by the xfs_has_attr routine above. If they happen at this
point, it would actually be more of an internal error. For example: if
we're renaming an attr, and xfs_has_attr finds that it exists, but then
xfs_attr_set_args comes back with ENOATTR, clearly something unexpected
happened.
The motivation for this is just that if it does happen, it's easier to
work out this out now rather than later when we bring in the rest of the
delayed attribute code. Because in that case, the error wont happen
here, it will happen later as part of a finish_item (or a log replay).
It's not a requirement I suppose, just more of a pro-active check really.
/me is in this funny place where he thinks that in general adding
asserts (or WARN_ON) to check assumptions is a good idea, but not sure
what's going on here.
Did that answer your question then?
--D
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
