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

Darrick J. Wong
2020-01-21T22:31:18Z

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

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

(I dunno, am I misunderstanding here?)

> Allison