[PATCH] Convert Samba to OFD locking - version 2

Mon May 23 14:26:42 UTC 2016

On Fri, 2016-05-20 at 17:07 -0700, Jeremy Allison wrote:
> On Fri, May 20, 2016 at 07:01:18AM -0400, Jeff Layton wrote:
> > On Thu, 2016-05-19 at 14:12 -0700, Jeremy Allison wrote:
> > > OK, here is version 2 with Uri and Simo's changes
> > > incorporated. Compile-time checks now run the test
> > > code rather than just compiling, and there's a
> > > tuneable "smbd:force process locks" that will force
> > > the old-style process specific locks. Advice to
> > > use it gets printed in the log if fcntl returns
> > > EINVAL.
> > > 
> > > Please review and push if happy !
> > > 
> > > Thanks,
> > > 
> > > Jeremy.
> > 
> > Looks reasonable to me, though again, I don't know the userland
> > samba
> > code that well. So, fwiw...
> > 
> >     Acked-by: Jeff Layton <jlayton at samba.org>
> > 
> > ...also attached is an untested patch for cifsfs, which I was
> > thinking
> > would do the right thing wrt lockowners. I'll test it out as soon
> > as I
> > can put a test rig together for it. In the meantime, thoughts?
> 
> Looks exactly right to me. With that and my patches
> OFD-locks should work over the wire to Samba with
> cifsfs and SMB1. You'll have to wait until my
> autobuild lands in master first though (2 unrelated
> and random different failures so far, grrr. I keep trying).
> 
> SMB2 uses the persistent part of the handle as the
> lock context so once we've gotten SMB2+ unix extensions
> onboard this should just work out of the box over
> SMB3.
> 

AFAICT, this patch makes no material difference in testing.

The client first compares the incoming lock request vs. the local lock
database. If there is already a conflicting lock set on the file, it's
rejected there before the lock request ever goes to the server. I guess
this makes sense from a performance standpoint -- no need to go out
over the network if we already know there's a conflicting lock.

This patch does seem more correct to me than sending the pid, given how
samba uses that field. But, I think we already have working OFD
semantics anyway, as long as the client's kernel supports them properly
for local filesystems.

Steve, I figure this is your call. Should I send it to linux-cifs? If
you do want it, there's certainly no urgency. It could soak in linux-
next for a cycle or two...

-- Jeff

> > From c976fce960bad85bb97ad62b7d3efc8fe04aafad Mon Sep 17 00:00:00
> > 2001
> > From: Jeff Layton <jeff.layton at primarydata.com>
> > Date: Fri, 20 May 2016 06:18:24 -0400
> > Subject: [PATCH] cifs: stuff the fl_owner into "pid" field in the
> > lock request
> > 
> > Right now, we send a "pid" across the wire. What we really want to
> > send
> > though is a hashed fl_owner_t. That should allow OFD locks to work
> > seamlessly in cifs with POSIX extensions.
> > 
> > Signed-off-by: Jeff Layton <jeff.layton at primarydata.com>
> > ---
> >  fs/cifs/cifsfs.c   |  3 +++
> >  fs/cifs/cifsglob.h |  1 +
> >  fs/cifs/file.c     | 14 +++++++++++---
> >  3 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 08fa36e5b2bc..1ef5b8aee878 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -87,6 +87,7 @@ extern mempool_t *cifs_req_poolp;
> >  extern mempool_t *cifs_mid_poolp;
> >  
> >  struct workqueue_struct	*cifsiod_wq;
> > +__u32 cifs_lock_secret;
> >  
> >  /*
> >   * Bumps refcount for cifs super block.
> > @@ -1271,6 +1272,8 @@ init_cifs(void)
> >  	spin_lock_init(&cifs_file_list_lock);
> >  	spin_lock_init(&GlobalMid_Lock);
> >  
> > +	get_random_bytes(&cifs_lock_secret,
> > sizeof(cifs_lock_secret));
> > +
> >  	if (cifs_max_pending < 2) {
> >  		cifs_max_pending = 2;
> >  		cifs_dbg(FYI, "cifs_max_pending set to min of
> > 2\n");
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> > index bba106cdc43c..8f1d8c1e72be 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -1619,6 +1619,7 @@ void cifs_oplock_break(struct work_struct
> > *work);
> >  
> >  extern const struct slow_work_ops cifs_oplock_break_ops;
> >  extern struct workqueue_struct *cifsiod_wq;
> > +extern __u32 cifs_lock_secret;
> >  
> >  extern mempool_t *cifs_mid_poolp;
> >  
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 9793ae0bcaa2..d4890b6dc22d 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -1112,6 +1112,12 @@ cifs_push_mandatory_locks(struct
> > cifsFileInfo *cfile)
> >  	return rc;
> >  }
> >  
> > +static __u32
> > +hash_lockowner(fl_owner_t owner)
> > +{
> > +	return cifs_lock_secret ^ hash32_ptr((const void *)owner);
> > +}
> > +
> >  struct lock_to_push {
> >  	struct list_head llist;
> >  	__u64 offset;
> > @@ -1178,7 +1184,7 @@ cifs_push_posix_locks(struct cifsFileInfo
> > *cfile)
> >  		else
> >  			type = CIFS_WRLCK;
> >  		lck = list_entry(el, struct lock_to_push, llist);
> > -		lck->pid = flock->fl_pid;
> > +		lck->pid = hash_lockowner(flock->fl_owner);
> >  		lck->netfid = cfile->fid.netfid;
> >  		lck->length = length;
> >  		lck->type = type;
> > @@ -1305,7 +1311,8 @@ cifs_getlk(struct file *file, struct
> > file_lock *flock, __u32 type,
> >  			posix_lock_type = CIFS_RDLCK;
> >  		else
> >  			posix_lock_type = CIFS_WRLCK;
> > -		rc = CIFSSMBPosixLock(xid, tcon, netfid, current-
> > >tgid,
> > +		rc = CIFSSMBPosixLock(xid, tcon, netfid,
> > +				      hash_lockowner(flock-
> > >fl_owner),
> >  				      flock->fl_start, length,
> > flock,
> >  				      posix_lock_type, wait_flag);
> >  		return rc;
> > @@ -1505,7 +1512,8 @@ cifs_setlk(struct file *file, struct
> > file_lock *flock, __u32 type,
> >  			posix_lock_type = CIFS_UNLCK;
> >  
> >  		rc = CIFSSMBPosixLock(xid, tcon, cfile-
> > >fid.netfid,
> > -				      current->tgid, flock-
> > >fl_start, length,
> > +				      hash_lockowner(flock-
> > >fl_owner),
> > +				      flock->fl_start, length,
> >  				      NULL, posix_lock_type,
> > wait_flag);
> >  		goto out;
> >  	}
> > -- 
> > 2.5.5
> > 
> 
-- 
Jeff Layton <jlayton at samba.org>