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

[Christoph Hellwig <hch@infradead.org>]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs