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

[Allison Collins <allison.henderson@oracle.com>]

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@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