lock status of dvp in lookup error return?

[Kostik Belousov]

On Wed, Oct 19, 2011 at 02:07:17AM -0400, Benjamin Kaduk wrote:
> Hi Rick,
> 
> In tracking down a panic trying to recursively lock a vnode in openafs, I 
> started questioning my behavior in the ISDOTDOT case, in particular 
> whether to drop the dvp lock before the actual call over the network; this 
> naturally led me to look at the NFS code as a reference.
> Unfortunately, this left me more confused than when I began ...
> 
> sys/fs/nfs_clvnops.c, in nfs_lookup():
>   1211          if (flags & ISDOTDOT) {
>   1212                  ltype = NFSVOPISLOCKED(dvp);
>   1213                  error = vfs_busy(mp, MBF_NOWAIT);
>   1214                  if (error != 0) {
>   1215                      vfs_ref(mp);
>   1216                      NFSVOPUNLOCK(dvp, 0);
>   1217                      error = vfs_busy(mp, 0);
>   1218                      NFSVOPLOCK(dvp, ltype | LK_RETRY);
> 
> If we fail to busy the mountpoint, drop the directory lock and try again, 
> then relock dvp afterward.
> 
>   1219                      vfs_rel(mp);
>   1220                      if (error == 0 && (dvp->v_iflag & VI_DOOMED)) {
>   1221                          vfs_unbusy(mp);
>   1222                          error = ENOENT;
>   1223                      }
>   1224                      if (error != 0)
>   1225                          return (error);
> 
> But if the second vfs_busy failed, or dvp is DOOMED, return with dvp 
> locked.
> 
>   1226                  }
>   1227                  NFSVOPUNLOCK(dvp, 0);
> 
> But now we always unlock dvp.
> 
>   1228                  error = nfscl_nget(mp, dvp, nfhp, cnp, td, &np, 
>   NULL,
>   1229                      cnp->cn_lkflags);
> 
> The call to the network (?)
> 
>   1230                  if (error == 0)
>   1231                      newvp = NFSTOV(np);
>   1232                  vfs_unbusy(mp);
>   1233                  if (newvp != dvp)
>   1234                      NFSVOPLOCK(dvp, ltype | LK_RETRY);
Did you missed line 1234 ?

The code is the copy of the vn_vget_ino(). The logic in the function
might be slightly easier to follow.

>   1235                  if (dvp->v_iflag & VI_DOOMED) {
>   1236                      if (error == 0) {
>   1237                          if (newvp == dvp)
>   1238                                  vrele(newvp);
>   1239                          else
>   1240                                  vput(newvp);
>   1241                      }
>   1242                      error = ENOENT;
>   1243                  }
>   1244                  if (error != 0)
>   1245                      return (error);
> 
> And here if there was an error hearing from the network, we return with 
> dvp still unlocked.
> 
>   1246                  if (attrflag)
>   1247                      (void) nfscl_loadattrcache(&newvp, &nfsva, 
>   NULL, NULL,
>   1248                              0, 1);
> 
> 
> So, I'm still confused about whether I should be unlocking dvp in the 
> error case for ISDOTDOT (though presumably looking at other filesystems 
> would help).  This inconsistency in the NFS client looks like a bug at my 
> current level of understanding -- what do you think?
> 
> Thanks,
> 
> Ben Kaduk
> _______________________________________________
> freebsd-fs at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20111019/453a9446/attachment.pgp