Message-ID: <1497989638.5054.3.camel@poochiereds.net>
Date:   Tue, 20 Jun 2017 16:13:58 -0400
From:   Jeff Layton <jlayton@...chiereds.net>
To:     Benjamin Coddington <bcodding@...hat.com>
Cc:     bfields@...ldses.org, Alexander Viro <viro@...iv.linux.org.uk>,
        linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] fs/locks: Remove fl_nspid and use fs-specific l_pid
 for remote locks

On Tue, 2017-06-20 at 15:39 -0400, Benjamin Coddington wrote:
> On 20 Jun 2017, at 15:32, Jeff Layton wrote:
> 
> > On Tue, 2017-06-20 at 15:17 -0400, Benjamin Coddington wrote:
> > > On 20 Jun 2017, at 13:06, Jeff Layton wrote:
> > > > 
> > > > Now that I think about it a bit more, I don't think we really need a
> > > > flag here.
> > > > 
> > > > Just have the ->lock operation set the fl_pid to a negative value. 
> > > > That
> > > > will never be a valid pid anyway. Then flock_translate_pid could 
> > > > just
> > > > return any negative value directly instead of trying to translate 
> > > > it.
> > > > 
> > > > In practice we would always just set it to -1. Maybe even add 
> > > > something
> > > > like this that the lock-> operation could set it to?
> > > > 
> > > > #define    FILE_LOCK_OWNER_UNDEFINED       -1
> > > 
> > > So for filesystems that set a remote pid, they should negate the pid 
> > > to mean
> > > that the pid should not be translated?  Then when we return that pid, 
> > > we
> > > flip it back again, or display a negative number, or turn it into -1?
> > > 
> > > The flag, having a readable name, would make things a bit clearer as 
> > > to what
> > > the filesystems expect to happen to that pid value.
> > > 
> > 
> > I now think that we really only ought to be filling out the pid when 
> > it
> > refers to a process on the local host. It seems sketchy to me to 
> > return
> > a pid here that is really the pid on another host, but happens to have
> > the same pid as something else on this host. It's misleading at best,
> > and if anyone tries to act on that info it could be dangerous. So I'm
> > thinking that we should just set it to -1 when the lock is held by
> > another host entirely.
> > 
> > But, since pid values must be positive, we can code the basic
> > infrastructure to return any negative value as-is instead of trying to
> > translate it.
> 
> Ok, so we have to patch several filesystems.  The question is do we 
> patch
> those filesystems that set remote pids to negate their pid values in the 
> lock
> they return from F_GETLK, or do we ask them to set a flag?  We'd be 
> patching
> them to negate their pid just to then transform it to -1..
> 
> I'd prefer a flag rather than carrying meaning in a modified value since 
> the
> flag has readable information.  No one will come along later and wonder 
> why
> some filesystems are negating their pid values.
> 
> If we're going to touch filesystems that set have remote locks anyway,
> perhaps it makes sense to take a step toward l_sysid by adding another
> member to file_lock.  Then a special value of fl_sysid would indicate 
> the
> local system.
> 

I think we need to fix up the current API first.

My main interest is that we have the kernel report l_pid properly to the
best of its ability, and when it can't that it report some clearly non-
sensical value (e.g., -1) for the pid. I think that's the only sane
thing we can do at this point.

If we want to start discussing new locking APIs then I'm fine with that,
but I'd still want to do something sane here before we start down that
road anyway.

-- 
Jeff Layton <jlayton@...chiereds.net>

Powered by blists - more mailing lists