Re: [PATCH v5 04/14] xfs: Add xfs_has_attr and subroutines

Allison Collins
2020-01-22T00:28:04Z

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@oracle.com>
>>>>
>>>> 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