XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Thu, Oct 16, 2008 at 07:50:10PM +0200, Miklos Szeredi wrote:
>> should be
>>
>>   		return -XFS_ERROR(-PTR_ERR(dentry));
> 
> No need for XFS_ERROR at all here, it's only used for error injection in
> low-level code.  Updated version that propagates the error below:
> 
> (Al, if you rebase vfs.git this should probably be folded into the patch
> that switches over to d_obtain_alias)
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

[...stuff deleted]

> Index: vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- vfs-2.6.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-15 21:26:33.000000000 +0200
> +++ vfs-2.6/fs/xfs/linux-2.6/xfs_ioctl.c	2008-10-16 20:06:20.000000000 +0200
> @@ -312,9 +312,9 @@ xfs_open_by_handle(
>  	}
>  
>  	dentry = d_obtain_alias(inode);
> -	if (dentry == NULL) {
> +	if (IS_ERR(dentry)) {
>  		put_unused_fd(new_fd);
> -		return -XFS_ERROR(ENOMEM);
> +		return PTR_ERR(dentry);
>  	}
>  
>  	/* Ensure umount returns EBUSY on umounts while this file is open. */
> --

Fair enough.
But XFS_ERROR is used throughout the function.
I've found the whole idea of when and when not to use XFS_ERROR annoying :)

I've never used it (other than calling it to stay consistent with the code).
Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] -
and it does this in xfs_error_trap().
Can one not decide to do this at any error point?
I can't see where we hook in to set up xfs_etrap.

> #ifdef DEBUG
> #define XFS_ERROR_NTRAP 10
> extern int      xfs_etrap[XFS_ERROR_NTRAP];
> extern int      xfs_error_trap(int);
> #define XFS_ERROR(e)    xfs_error_trap(e)
> #else
> #define XFS_ERROR(e)    (e)
> #endif


Cheers,
Tim.

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Dave Chinner]

On Fri, Oct 17, 2008 at 11:11:00AM +1100, Timothy Shimmin wrote:
> Fair enough.
> But XFS_ERROR is used throughout the function.
> I've found the whole idea of when and when not to use XFS_ERROR annoying :)
> 
> I've never used it (other than calling it to stay consistent with the code).
> Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] -
> and it does this in xfs_error_trap().
> Can one not decide to do this at any error point?
> I can't see where we hook in to set up xfs_etrap.

You break into the debugger, modify the xfs_etrap array to contain
the set of errors you want to catch, then continue onwards.  I've
used this several times in the past couple of months to locate the
source of strange ENOSPC errors...

The conceptual idea is that every point where an error is first
detected gets wrapped with XFS_ERROR() so you don't need to sprinkle
printk's all through the code to find where the error is coming
from.

ISTR the Irix kernel debugger had a command to set the values
in the etrap array - if it did it never got ported to kdb....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Eric Sandeen]

Dave Chinner wrote:
> On Fri, Oct 17, 2008 at 11:11:00AM +1100, Timothy Shimmin wrote:
>> Fair enough.
>> But XFS_ERROR is used throughout the function.
>> I've found the whole idea of when and when not to use XFS_ERROR annoying :)
>>
>> I've never used it (other than calling it to stay consistent with the code).
>> Looking at the code, it is used to BUG and print a msg on particular error codes set in xfs_etrap[] -
>> and it does this in xfs_error_trap().
>> Can one not decide to do this at any error point?
>> I can't see where we hook in to set up xfs_etrap.
> 
> You break into the debugger, modify the xfs_etrap array to contain
> the set of errors you want to catch, then continue onwards.

bleah :)

Could these just be turned into tracepoints eventually?  Or maybe for
now allow modifying the array via proc or something... would be easier
to ask a user to do that if you don't have direct access to their kdb
console ;)  It is a handy thing to have, though.

-Eric

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

> 
> Fair enough.
> But XFS_ERROR is used throughout the function.

Can we leave it the simple way for now?  I have to revamp that whole
function anyway as it's extremly buggy in many ways, especially when
used to open directories (can lead to multiple dentries for a single
directory - ouch) and then I'll kill the other uses.

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Timothy Shimmin]

Christoph Hellwig wrote:
>> Fair enough.
>> But XFS_ERROR is used throughout the function.
> 
> Can we leave it the simple way for now?  

Yep.

> I have to revamp that whole
> function anyway as it's extremly buggy in many ways, especially when
> used to open directories (can lead to multiple dentries for a single
> directory - ouch) and then I'll kill the other uses.

Oh ok.

In userspace,
we use it for opening directories on xfsdump via jdm_open in order to
do bulkstat driven dirent dumping.
We also use it in xfsrestore - though I am not convinced we should -
it was initially done for "performance" reasons apparently.

--Tim

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Christoph Hellwig]

On Mon, Oct 20, 2008 at 11:49:47AM +1100, Timothy Shimmin wrote:
> > I have to revamp that whole
> > function anyway as it's extremly buggy in many ways, especially when
> > used to open directories (can lead to multiple dentries for a single
> > directory - ouch) and then I'll kill the other uses.
> 
> Oh ok.
> 
> In userspace,
> we use it for opening directories on xfsdump via jdm_open in order to
> do bulkstat driven dirent dumping.
> We also use it in xfsrestore - though I am not convinced we should -
> it was initially done for "performance" reasons apparently.

I think the use of the API is fine, the problem is that the current
implemention is buggy.

Re: XFS_ERROR use - was Re: [PATCH] prevent NULL returns from d_obtain_alias

[Timothy Shimmin]

Christoph Hellwig wrote:
> On Mon, Oct 20, 2008 at 11:49:47AM +1100, Timothy Shimmin wrote:
>>> I have to revamp that whole
>>> function anyway as it's extremly buggy in many ways, especially when
>>> used to open directories (can lead to multiple dentries for a single
>>> directory - ouch) and then I'll kill the other uses.
>> Oh ok.
>>
>> In userspace,
>> we use it for opening directories on xfsdump via jdm_open in order to
>> do bulkstat driven dirent dumping.
>> We also use it in xfsrestore - though I am not convinced we should -
>> it was initially done for "performance" reasons apparently.
> 
> I think the use of the API is fine, the problem is that the current
> implemention is buggy.
Yeah, I have no problem for xfsdump as it is driven by bulkstat
but for xfsrestore it seems unnecessary.
In fact, it can restore to other filesystems so it must only be using
this in certain ways on restore.
Having a look, yeah it is predicated by:
  if ( tranp->t_dstdirisxfspr )
and then just uses the fd for XFS_IOC_FSSETDM, XFS_IOC_FSSETXATTR
for those xfs-specific things and in the same function it uses
the path for non-xfs-specific: utime, chown, chmod
So it seems unnecessary to me in restore.

--Tim