Re: repeatable ZFS panic: share->excl

On Sunday 29 March 2009 12:29:13 pm Tim Kientzle wrote:
> Pawel Jakub Dawidek wrote:
> > On Fri, Mar 13, 2009 at 02:08:03PM -0400, John Baldwin wrote:
> >> John Baldwin wrote:
> >>> Yes, I think that is the real bug.  Looking at this further I think
> >>> zfs_get_xattrdir() will return the vnode locked if it has to create a
> >>> new node via zfs_make_attrdir() but only returns it held and unlocked if
> >>> it finds an existing one.  So my new patch is to just fix
> >>> zfs_get_xattrdir() to unlock the vnode if it creates a new one like so:
> >>>
> >>> (Sorry, TBird is probably going to butcher all the whitespace):
> >>>
> >>> ---
> 
>>> //depot/user/jhb/lock/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c
> >>> +++
> 
>>> /Users/jhb/work/p4/lock/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_dir.c 
> >>>
> >>> _at__at_ -940,6 +940,7 _at__at_
> >>>         /* NB: we already did dmu_tx_wait() if necessary */
> >>>         goto top;
> >>>     }
> >>> +    VOP_UNLOCK(*xvpp, 0);
> >>>
> >>>     return (error);
> >>> }
> >>>
> >>> A non-butchered version is at www.FreeBSD.org/~jhb/patches/zfs_ea.patch.
> >> So lulf_at_ reports success with this patch.  Pawel, can you review it?
> > 
> > Yes, it works for me too and looks good. The only thing we need to
> > change is to check for error beeing 0 before unlocking the vnode.
> > The zfs_make_xattrdir() function can still return with EIO, so I'd add
> > something like this:
> > 
> > 	if (error == 0)
> > 		VOP_UNLOCK(*xvpp, 0);
> > 
> > Thank you John for spending time on tracking this one down.
> 
> Any estimate on when this can be committed?
> 
> I'm waiting to re-enable the extended attribute
> archiving support in tar until this is fixed.

Err, it went into the tree a while ago:

Author: jhb
Date: Wed Mar 18 16:19:44 2009
New Revision: 189967
URL: http://svn.freebsd.org/changeset/base/189967

Log:
  The zfs_get_xattrdir() function is used to find the extended attribute
  directory for a znode.  When the directory already exists, it returns a
  referenced but unlocked vnode.  When a directory does not yet exist, it
  calls zfs_make_xattrdir() to create a new one.  zfs_make_xattrdir() returns
  the vnode both referenced and and locked and zfs_get_xattrdir() was leaking
  this vnode lock to its callers.  Fix this by dropping the vnode lock if
  zfs_make_xattrdir() successfully creates a new extended attribute
  directory.
  
  Reviewed by:  pjd

-- 
John Baldwin