[PATCH] locks: export device name

[PATCH] locks: export device name

[Davidlohr Bueso]

From: Davidlohr Bueso <dave@gnu.org>

The lslk(8) program has not been maintained for over a decade and has recently been rewritten as lslocks(8).
It will be available for the next 2.22 release, in a couple of months. This is a good opportunity to delete
that nasty WE_CAN_BREAK_LSLK_NOW and start exporting the device name instead of the maj:min numbers.

For backward compatibility the new version can be in charge of checking older kernel versions and parsing the old
output if necessary.

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 fs/locks.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 637694b..2b01907 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2199,15 +2199,8 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
 			       : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
 	}
 	if (inode) {
-#ifdef WE_CAN_BREAK_LSLK_NOW
 		seq_printf(f, "%d %s:%ld ", fl_pid,
 				inode->i_sb->s_id, inode->i_ino);
-#else
-		/* userspace relies on this representation of dev_t ;-( */
-		seq_printf(f, "%d %02x:%02x:%ld ", fl_pid,
-				MAJOR(inode->i_sb->s_dev),
-				MINOR(inode->i_sb->s_dev), inode->i_ino);
-#endif
 	} else {
 		seq_printf(f, "%d <none>:0 ", fl_pid);
 	}
-- 
1.7.4.1

Re: [PATCH] locks: export device name

[Andrew Morton]

On Fri, 10 Feb 2012 22:06:07 +0100
Davidlohr Bueso <dave@gnu.org> wrote:

> From: Davidlohr Bueso <dave@gnu.org>
> 
> The lslk(8) program has not been maintained for over a decade and has recently been rewritten as lslocks(8).
> It will be available for the next 2.22 release, in a couple of months. This is a good opportunity to delete
> that nasty WE_CAN_BREAK_LSLK_NOW and start exporting the device name instead of the maj:min numbers.
> 
> For backward compatibility the new version can be in charge of checking older kernel versions and parsing the old
> output if necessary.
> 
> ...
>
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -2199,15 +2199,8 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
>  			       : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
>  	}
>  	if (inode) {
> -#ifdef WE_CAN_BREAK_LSLK_NOW
>  		seq_printf(f, "%d %s:%ld ", fl_pid,
>  				inode->i_sb->s_id, inode->i_ino);
> -#else
> -		/* userspace relies on this representation of dev_t ;-( */
> -		seq_printf(f, "%d %02x:%02x:%ld ", fl_pid,
> -				MAJOR(inode->i_sb->s_dev),
> -				MINOR(inode->i_sb->s_dev), inode->i_ino);
> -#endif
>  	} else {
>  		seq_printf(f, "%d <none>:0 ", fl_pid);
>  	}

I don't get it.  This is an immediate and non-back-compatible change to
the format of /proc/locks.  The only way this can avoid breaking things
is if there are no programs or scripts in use by anyone which use
this field.  What am I missing here?

Re: [PATCH] locks: export device name

[J. Bruce Fields]

On Mon, Feb 13, 2012 at 04:34:25PM -0800, Andrew Morton wrote:
> On Fri, 10 Feb 2012 22:06:07 +0100
> Davidlohr Bueso <dave@gnu.org> wrote:
> 
> > From: Davidlohr Bueso <dave@gnu.org>
> > 
> > The lslk(8) program has not been maintained for over a decade and has recently been rewritten as lslocks(8).
> > It will be available for the next 2.22 release, in a couple of months. This is a good opportunity to delete
> > that nasty WE_CAN_BREAK_LSLK_NOW and start exporting the device name instead of the maj:min numbers.
> > 
> > For backward compatibility the new version can be in charge of checking older kernel versions and parsing the old
> > output if necessary.
> > 
> > ...
> >
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -2199,15 +2199,8 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> >  			       : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
> >  	}
> >  	if (inode) {
> > -#ifdef WE_CAN_BREAK_LSLK_NOW
> >  		seq_printf(f, "%d %s:%ld ", fl_pid,
> >  				inode->i_sb->s_id, inode->i_ino);
> > -#else
> > -		/* userspace relies on this representation of dev_t ;-( */
> > -		seq_printf(f, "%d %02x:%02x:%ld ", fl_pid,
> > -				MAJOR(inode->i_sb->s_dev),
> > -				MINOR(inode->i_sb->s_dev), inode->i_ino);
> > -#endif
> >  	} else {
> >  		seq_printf(f, "%d <none>:0 ", fl_pid);
> >  	}
> 
> I don't get it.  This is an immediate and non-back-compatible change to
> the format of /proc/locks.  The only way this can avoid breaking things
> is if there are no programs or scripts in use by anyone which use
> this field.  What am I missing here?

I'm a little surprised anything parses that file.

But, yes, looks like I can "yum install" lslk on Fedora 16, as an
example.  Can't get it to do anything useful, though.  Does it actually
work on any recent distro?

Perhaps safest would be to replace /proc/locks by another interface and
deprecate this one.

--b.

Re: [PATCH] locks: export device name

[Davidlohr Bueso]

On Tue, 2012-02-14 at 14:09 -0500, J. Bruce Fields wrote:
> On Mon, Feb 13, 2012 at 04:34:25PM -0800, Andrew Morton wrote:
> > On Fri, 10 Feb 2012 22:06:07 +0100
> > Davidlohr Bueso <dave@gnu.org> wrote:
> > 
> > > From: Davidlohr Bueso <dave@gnu.org>
> > > 
> > > The lslk(8) program has not been maintained for over a decade and has recently been rewritten as lslocks(8).
> > > It will be available for the next 2.22 release, in a couple of months. This is a good opportunity to delete
> > > that nasty WE_CAN_BREAK_LSLK_NOW and start exporting the device name instead of the maj:min numbers.
> > > 
> > > For backward compatibility the new version can be in charge of checking older kernel versions and parsing the old
> > > output if necessary.
> > > 
> > > ...
> > >
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -2199,15 +2199,8 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> > >  			       : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
> > >  	}
> > >  	if (inode) {
> > > -#ifdef WE_CAN_BREAK_LSLK_NOW
> > >  		seq_printf(f, "%d %s:%ld ", fl_pid,
> > >  				inode->i_sb->s_id, inode->i_ino);
> > > -#else
> > > -		/* userspace relies on this representation of dev_t ;-( */
> > > -		seq_printf(f, "%d %02x:%02x:%ld ", fl_pid,
> > > -				MAJOR(inode->i_sb->s_dev),
> > > -				MINOR(inode->i_sb->s_dev), inode->i_ino);
> > > -#endif
> > >  	} else {
> > >  		seq_printf(f, "%d <none>:0 ", fl_pid);
> > >  	}
> > 
> > I don't get it.  This is an immediate and non-back-compatible change to
> > the format of /proc/locks.  The only way this can avoid breaking things
> > is if there are no programs or scripts in use by anyone which use
> > this field.  What am I missing here?
> 
> I'm a little surprised anything parses that file.

To my knowledge only lslk - but the whole point here is that its going
to be replaced by lslocks.

> 
> But, yes, looks like I can "yum install" lslk on Fedora 16, as an
> example.  Can't get it to do anything useful, though.  Does it actually
> work on any recent distro?

It works on Ubuntu's latest release.

> 
> Perhaps safest would be to replace /proc/locks by another interface and
> deprecate this one.

If exporting the name in the current /proc/locks file is out of the
question, then IMHO I don't think it would be worth adding a new
interface just for such a small change.

Thanks,
Davidlohr

Re: [PATCH] locks: export device name

[J. Bruce Fields]

On Wed, Feb 15, 2012 at 11:52:42AM +0100, Davidlohr Bueso wrote:
> On Tue, 2012-02-14 at 14:09 -0500, J. Bruce Fields wrote:
> > On Mon, Feb 13, 2012 at 04:34:25PM -0800, Andrew Morton wrote:
> > > I don't get it.  This is an immediate and non-back-compatible change to
> > > the format of /proc/locks.  The only way this can avoid breaking things
> > > is if there are no programs or scripts in use by anyone which use
> > > this field.  What am I missing here?
> > 
> > I'm a little surprised anything parses that file.
> 
> To my knowledge only lslk - but the whole point here is that its going
> to be replaced by lslocks.
> 
> > 
> > But, yes, looks like I can "yum install" lslk on Fedora 16, as an
> > example.  Can't get it to do anything useful, though.  Does it actually
> > work on any recent distro?
> 
> It works on Ubuntu's latest release.

OK, in that case I'm with Andrew, we'd need to do this more carefully.

People should be able to use something like a recent Ubuntu release to
test more recent kernels, and we don't want their tools to break when we
do that.

> > Perhaps safest would be to replace /proc/locks by another interface and
> > deprecate this one.
> 
> If exporting the name in the current /proc/locks file is out of the
> question, then IMHO I don't think it would be worth adding a new
> interface just for such a small change.

OK.

If you want to just change this over, I guess the thing to do would be
to stick something in feature-removal-schedule.txt saying "we'll switch
this in 2 years" (or however long you think before there are
realistically no more lslk users left), then do it then.

Switching to a new api would be better as we could warn users of the old
api then.  Maybe it'd be worth it if there was some other change we'd
been wanting to make?   Can't think of anything off the top of my head.

We may be adding more lock types--will lslk and lslocks handle that
gracefully?

--b.

Re: [PATCH] locks: export device name

[Andrew Morton]

On Wed, 15 Feb 2012 07:42:30 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> > > Perhaps safest would be to replace /proc/locks by another interface and
> > > deprecate this one.
> > 
> > If exporting the name in the current /proc/locks file is out of the
> > question, then IMHO I don't think it would be worth adding a new
> > interface just for such a small change.
> 
> OK.
> 
> If you want to just change this over, I guess the thing to do would be
> to stick something in feature-removal-schedule.txt saying "we'll switch
> this in 2 years" (or however long you think before there are
> realistically no more lslk users left), then do it then.
> 
> Switching to a new api would be better as we could warn users of the old
> api then.  Maybe it'd be worth it if there was some other change we'd
> been wanting to make?   Can't think of anything off the top of my head.
> 
> We may be adding more lock types--will lslk and lslocks handle that
> gracefully?

Adding a whole new interface is pretty attractive.  It lets us get it
right this time.  In particular, something which is extensible given
certain simple rules.  As we've learned, the current /proc/locks didn't
get that right!

We can eventually remove the old code - it may take longer than two
years, but whatever.  If we go this way, we should arrange for the
kernel to emit a warning (printk_once) into the logs the first time
someone accesses the old file.  This will help to prompt people to
migrate off the deprecated interface.  After a while, we can add a
config option to make the old interface go away.  Distros will start to
disable the feature.  Later, we zap it altogether.

Re: [PATCH] locks: export device name

[Davidlohr Bueso]

On Wed, 2012-02-15 at 12:39 -0800, Andrew Morton wrote:
> On Wed, 15 Feb 2012 07:42:30 -0500
> "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> > > > Perhaps safest would be to replace /proc/locks by another interface and
> > > > deprecate this one.
> > > 
> > > If exporting the name in the current /proc/locks file is out of the
> > > question, then IMHO I don't think it would be worth adding a new
> > > interface just for such a small change.
> > 
> > OK.
> > 
> > If you want to just change this over, I guess the thing to do would be
> > to stick something in feature-removal-schedule.txt saying "we'll switch
> > this in 2 years" (or however long you think before there are
> > realistically no more lslk users left), then do it then.
> > 
> > Switching to a new api would be better as we could warn users of the old
> > api then.  Maybe it'd be worth it if there was some other change we'd
> > been wanting to make?   Can't think of anything off the top of my head.
> > 
> > We may be adding more lock types--will lslk and lslocks handle that
> > gracefully?
> 
> Adding a whole new interface is pretty attractive.  It lets us get it
> right this time.  In particular, something which is extensible given
> certain simple rules.  As we've learned, the current /proc/locks didn't
> get that right!

Ok, however I'm a bit confused on what you mean by extensible; since
what we decide to export to userspace is pretty much permanent, how can
we change (extend) it later? We'd pretty much be running into
the /proc/locks situation now.

> 
> We can eventually remove the old code - it may take longer than two
> years, but whatever.  If we go this way, we should arrange for the
> kernel to emit a warning (printk_once) into the logs the first time
> someone accesses the old file.  This will help to prompt people to
> migrate off the deprecated interface.  After a while, we can add a
> config option to make the old interface go away.  Distros will start to
> disable the feature.  Later, we zap it altogether.

Kind if like what was done with the /proc/x/oom_adj interface.

Re: [PATCH] locks: export device name

[Andrew Morton]

On Thu, 16 Feb 2012 23:37:11 +0100
Davidlohr Bueso <dave@gnu.org> wrote:

> On Wed, 2012-02-15 at 12:39 -0800, Andrew Morton wrote:
> > On Wed, 15 Feb 2012 07:42:30 -0500
> > "J. Bruce Fields" <bfields@fieldses.org> wrote:
> > 
> > > > > Perhaps safest would be to replace /proc/locks by another interface and
> > > > > deprecate this one.
> > > > 
> > > > If exporting the name in the current /proc/locks file is out of the
> > > > question, then IMHO I don't think it would be worth adding a new
> > > > interface just for such a small change.
> > > 
> > > OK.
> > > 
> > > If you want to just change this over, I guess the thing to do would be
> > > to stick something in feature-removal-schedule.txt saying "we'll switch
> > > this in 2 years" (or however long you think before there are
> > > realistically no more lslk users left), then do it then.
> > > 
> > > Switching to a new api would be better as we could warn users of the old
> > > api then.  Maybe it'd be worth it if there was some other change we'd
> > > been wanting to make?   Can't think of anything off the top of my head.
> > > 
> > > We may be adding more lock types--will lslk and lslocks handle that
> > > gracefully?
> > 
> > Adding a whole new interface is pretty attractive.  It lets us get it
> > right this time.  In particular, something which is extensible given
> > certain simple rules.  As we've learned, the current /proc/locks didn't
> > get that right!
> 
> Ok, however I'm a bit confused on what you mean by extensible; since
> what we decide to export to userspace is pretty much permanent, how can
> we change (extend) it later? We'd pretty much be running into
> the /proc/locks situation now.

Mainly by avoiding the use of implicit identification via fixed
positions.  Look at /proc/stat and weep.

If we use a name:value format then we can add new fields later and
things work OK.  We add stuff to /proc/meminfo and /proc/vmstat all the
time.

Removing things is of course much harder.  The best fix is to avoid
adding things which we might ever have a reason for removing!  If we
have a field which we simply can no longer support and which we think
we must retain for back-compat reasons then we just have to find some
way to emulate it.  In extremis we could hardwire the value to "0" so
tools won't crash.

sysfs has a different convention: one-value-per-file.  That means
there's no need for the name part of name:value.  Extensibility means
"go add another sysfs file".