On Fri, 18 Jul 2014 08:54:02 -0700
Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> > +out_unlock:
> > + spin_unlock(&fp->fi_lock);
> > + spin_unlock(&state_lock);
> > +out_fput:
> > + if (filp)
> > + fput(filp);
>
> I don't think fput can be NULL here.
>
Correct. I'll fix that...
> > static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
> > {
> > + int status = 0;
> > +
> > if (fp->fi_had_conflict)
> > return -EAGAIN;
> > get_nfs4_file(fp);
> > + spin_lock(&state_lock);
> > + spin_lock(&fp->fi_lock);
> > dp->dl_file = fp;
> > + if (!fp->fi_lease) {
> > + spin_unlock(&fp->fi_lock);
> > + spin_unlock(&state_lock);
> > return nfs4_setlease(dp);
> > + }
> > atomic_inc(&fp->fi_delegees);
> > if (fp->fi_had_conflict) {
> > + status = -EAGAIN;
> > + goto out_unlock;
> > }
> > hash_delegation_locked(dp, fp);
> > +out_unlock:
> > + spin_unlock(&fp->fi_lock);
> > spin_unlock(&state_lock);
> > + return status;
>
> I have to admit that I didn't have time to go through all the
> surrounding code yet, but is there error handling correct here,
> i.e. no need to rop the file reference and cleanup dp->dl_file for any
> error or race case?
>
No, we take a reference to the nfs4_file and then immediately set
dl_file. At that point, once we put the delegation's final reference it
will put the file reference.
So, I *think* the error handling is now correct, but please do sanity
check me on this if you're able.
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
