I was thinking of adapting this patch to be part of a 3 patch series
including one of Chistophs. Something like this:
[PATCH v6 11/16] xfs: Check for -ENOATTR or -EEXIST
[PATCH v6 03/16] xfs: Add xfs_has_attr and subroutines
[PATCH 02/29] xfs: merge xfs_attr_remove into xfs_attr_set
What would people think of that? I figured this would be better than
the two of us bombarding the mailing list with giant conflicting sets?
Also, was I able to answer everyones questions on this patch?
Thanks!
Allison
On 1/21/20 5:25 PM, Allison Collins wrote:
On 1/21/20 3:30 PM, Darrick J. Wong wrote:
On Tue, Dec 24, 2019 at 09:21:49PM -0700, Allison Collins wrote:
On 12/24/19 5:18 AM, Christoph Hellwig wrote:
On Wed, Dec 11, 2019 at 09:15:03PM -0700, Allison Collins wrote:
From: Allison Henderson <allison.henderson@xxxxxxxxxx>
This patch adds a new functions to check for the existence of
an attribute. Subroutines are also added to handle the cases
of leaf blocks, nodes or shortform. Common code that appears
in existing attr add and remove functions have been factored
out to help reduce the appearance of duplicated code. We will
need these routines later for delayed attributes since delayed
operations cannot return error codes.
Can you explain why we need the ahead of time check? The first
operation should be able to still return an error, and doing
a separate check instead of letting the actual operation fail
gracefully is more expensive, and also creates a lot of additional
code. As is I can't say I like the direction at all.
This one I can answer quickly: later when we get into delayed
attributes,
this will get called from xfs_defer_finish_noroll as part of a
.finish_item
call back. If these callbacks return anything other than 0 or
-EAGAIN, it
causes a shutdown.
When does this happen, exactly?
Sure, lets take a look at fs/xfs/libxfs/xfs_defer.c line 399
We have the callback invocation:
error = ops->finish_item(*tp, li, dfp->dfp_done, &state);
This is where the intent items get to finish out their operations. If
they return -EAGAIN, they get get put back in the list, but if they
error out, we jump to the error handler (line 445), where we abort the
transaction and do a shutdown.
Now, if we peek ahead at the first patch that belongs to the next
delayed attribute series:
https://github.com/allisonhenderson/xfs_work/commit/7d5b7395fe3b1df7739089c0643e20b09de2e0b0
If you expand the diff for fs/xfs/xfs_attr_item.c, and zip down to line
269, you'll see a ".finish_item = xfs_attr_finish_item". This
callback will do some leg work to unwrap the log item and feed the
appropriate parameters back into xfs_attr_set_iter. So this is why we
go through all the trouble to check for any expected errors ahead of time.
Are you saying that during log
recovery, we can end up replaying a delayed attr log item that hits
ENOATTR/EEXIST somewhere and passes that out, which causes log recovery
to fail?
Well, the idea is to avoid that with this helper function. Reading
ahead, it looks like you've already connected the two :-)
Which is not what we would want for example: when the
user tries to rename a non-existent attribute. The error code needs
to go
back up. So we check for things like that before starting a delayed
operation. Hope that helps. Thanks!
...because as far as requests from user programs goes, we should be
doing all these precondition checks after allocating a transaction and
ILOCKing the inode, so that we can send the error code back to userspace
without cancelling a dirty transaction.
Yes, we do that later in the set, but I think you've got it ;-)
Allison
(I dunno, am I misunderstanding here?)
Allison
