Re: [PATCH 08/10] locks: move i_lock acquisition into generic_*_lease handlers

[Jeff Layton <jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>]

On Sun, 24 Aug 2014 09:06:34 -0700
Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> 
> Some comments on further work I'd like to see in this area, though:
> 
> > +	spin_lock(&inode->i_lock);
> > +	time_out_leases(inode);
> >  	for (before = &inode->i_flock;
> >  			((fl = *before) != NULL) && IS_LEASE(fl);
> >  			before = &fl->fl_next) {
> >  		if (fl->fl_file != filp)
> >  			continue;
> > -		return fl->fl_lmops->lm_change(before, F_UNLCK);
> > +		error = fl->fl_lmops->lm_change(before, F_UNLCK);
> >  	}
> 
> We really should split a lm_release from lm_change, the way it is
> used is highly confusing.  In addition I think a lot of code
> currently in lease_modify should move here instead, e.g. something like:
> 
> 
> 	if (fl->fl_file != filp)
> 		continue;
> 
> 	fl = *before;
> 	fl->fl_type = F_UNLCK;
> 	lease_clear_pending(fl, F_UNLCK);
> 	locks_wake_up_blocks(fl);
> 	if (fl->fl_lmops->lm_delete)
> 		fl->fl_lmops->lm_delete(fl);
> 	locks_delete_lock(before, NULL);
> 
> with lm_delete for user space leases as:
> 
> static void lease_delete(struct file_lock *fl)
> {
> 	struct file *filp = fl->fl_file;
> 
> 	f_delown(filp);
> 	filp->f_owner.signum = 0;
> 	fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
> 	if (fl->fl_fasync != NULL) {
> 		printk(KERN_ERR "locks_delete_lock: fasync == %p\n",
> 				fl->fl_fasync);
> 		fl->fl_fasync = NULL;
> 	}
> }
> 
> and a NULL implementation for delegations.

Good idea. I'll spin that up on the next iteration.

Thanks,
-- 
Jeff Layton <jlayton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html