[Top] [All Lists]

Re: [PATCH 2/2] xfs: xfs_attr_inactive leaves inconsistent attr fork sta

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: xfs_attr_inactive leaves inconsistent attr fork state behind
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 5 May 2015 22:02:08 -0700
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1430780408-12539-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1430780408-12539-1-git-send-email-david@xxxxxxxxxxxxx> <1430780408-12539-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
> -STATIC void
> +void
>  xfs_attr_fork_reset(

Maybe rename it to xfs_attr_fork_remove while you're at it?

> +     xfs_ilock(dp, lock_mode);
> +     if (!XFS_IFORK_Q(dp))
> +             goto out_destroy_fork;
> +     xfs_iunlock(dp, lock_mode);

The use of a goto here seems confsing as it moves the code to just
free the attribute code out of line like some error handling.

It could also use a comment on when we have an in-memory attribute
fork but XFS_IFORK_Q is false.  I don't really know when that
would be true given that xfs_attr_shortform_remove either removes
the attribute fork, or asserts that the forkoff is non-zero when
it is left as-is.

>       /*
> -      * Decide on what work routines to call based on the inode size.
> +      * It's unlikely we've raced with an attribute fork creation, but check
> +      * anyway just in case.
>        */

We always need to check for races if they are possible, no matter how
unlikely they are.  So that just in case comment seems confusing.

> +     if (XFS_IFORK_Q(ip)) {
>               error = xfs_attr_inactive(ip);
>               if (error)
>                       return;
>       }

Given that we don't even call xfs_attr_inactive when XFS_IFORK_Q is
false the check above doesn't seem to be reachable anyway.  At least
I can't think of a way how we could add an attr fork in a way that
races with inode teardown.

<Prev in Thread] Current Thread [Next in Thread>