8806 xattr_dir_inactive() releases used vnode with kernel panic

Review Request #789 — Created Dec. 4, 2017 and submitted

vgusev
illumos-gate
master
8806
3ae7d8f...
general
gwr
8806 xattr_dir_inactive() releases used vnode with kernel panic

Testing for 12+ hours based on reproducer at #8806, no crashes.

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
gwr
  1. Otherwise looks good.

  2. usr/src/uts/common/fs/xattr.c (Diff revision 1)
     
     

    A comment here would be nice. Maybe:

    Note: gfs_dir_inactive returns NULL when we've
    lost a race with VGET (node is still active)
    otherwise this is really the last ref.

    1. Let me just add comment " vp was freed "

    2. It is common usage of gfs_dir_inactive().

  3. 
      
vgusev
marcel
  1. 
      
  2. usr/src/uts/common/fs/xattr.c (Diff revision 2)
     
     

    Please add != NULL here.

  3. usr/src/uts/common/fs/xattr.c (Diff revision 2)
     
     

    I'm not very happy with this typecast, but explicit dp->xattr_gfs_private.gfsd_file.gfs_size would be harder to read (but I still prefer it).

    1. Or you could do something closer to what this consumer does:
      http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/fs/zfs/zfs_ctldir.c#1138
      and use sizeof (xattr_dir_t)

      That's the size we passed to gfs_dir_create.

  4. 
      
vgusev
  1. 
      
  2. usr/src/uts/common/fs/xattr.c (Diff revision 2)
     
     

    Please let me leave as is because original code is:

    kmem_free(fp, fp->gfs_size);

    gfs_size should be used here, not direct size because gfs_size is set in gfs layer and in xattr layer it could be different.

  3. 
      
vgusev
gwr
  1. Ship It!
  2. 
      
marcel
  1. Ship It!
  2. 
      
kmays
  1. Ship It!
  2. 
      
vgusev
Review request changed

Status: Closed (submitted)

Loading...