locks: export device name

PATCH In review Davidlohr Bueso 3 participants 8 messages Feb 10, 2012 – Feb 16, 2012

Subsystems (2)

VFS File locking (flock() and fcntl()/lockf())

lore

Share & tools

lore

Davidlohr Bueso

author

2012-02-10 21:06 UTC #0 lore raw

PATCH 1/1 locks: export device name

From: Davidlohr Bueso <[email protected]>

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 <[email protected]>

---
 fs/locks.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

fs/locks.c +0 -7

index 637694b..2b01907 100644
@@ -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);
 	}

Signature · 1 line

1.7.4.1

Andrew Morton

2012-02-14 00:34 UTC #1 lore raw

Quoted 19 lines · On Fri, 10 Feb 2012 22:06:07 +0100 Davidlohr Bueso <[email protected]> wrote:

From: Davidlohr Bueso <[email protected]>
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.
...
fs/locks.c +0 -1
@@ -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,

fs/locks.c static void lock_get_status(struct seq_file *f, struct file_lock *fl, · line 2206

Quoted 10 lines

fs/locks.c +0 -6

 				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?

J. Bruce Fields

2012-02-14 19:09 UTC #2 lore raw

Quoted 29 lines · Andrew Morton

Quoted 26 lines · On Fri, 10 Feb 2012 22:06:07 +0100 Davidlohr Bueso <[email protected]> wrote:
From: Davidlohr Bueso <[email protected]>
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.
...
fs/locks.c +0 -7
@@ -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

Quoted 8 lines

fs/locks.c static void lock_get_status(struct seq_file *f, struct file_lock *fl, · line 2206
Quoted 3 lines
fs/locks.c +0 -0
 	} 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.

Davidlohr Bueso

author

2012-02-15 10:52 UTC #3 lore raw

Quoted 39 lines · J. Bruce Fields

Quoted 29 lines · Andrew Morton
Quoted 26 lines · On Fri, 10 Feb 2012 22:06:07 +0100 Davidlohr Bueso <[email protected]> wrote:
From: Davidlohr Bueso <[email protected]>
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.
...
fs/locks.c +0 -7
@@ -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
Quoted 8 lines
fs/locks.c static void lock_get_status(struct seq_file *f, struct file_lock *fl, · line 2206
Quoted 3 lines
fs/locks.c +0 -0
 	} 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?

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 3 lines

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

J. Bruce Fields

2012-02-15 12:42 UTC #4 lore raw

Quoted 13 lines · Davidlohr Bueso

Quoted 7 lines · J. Bruce Fields
Quoted 4 lines · Andrew Morton
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.
Quoted 1 line

Quoted 5 lines

Quoted 3 lines
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.

Quoted 6 lines

Quoted 2 lines
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.

Andrew Morton

2012-02-15 20:39 UTC #5 lore raw

Quoted 14 lines · On Wed, 15 Feb 2012 07:42:30 -0500 "J. Bruce Fields" <[email protected]>…

Quoted 6 lines
Quoted 2 lines
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.

Quoted 6 lines

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.

Davidlohr Bueso

author

2012-02-16 22:37 UTC #6 lore raw

Quoted 21 lines · Andrew Morton

Quoted 18 lines · On Wed, 15 Feb 2012 07:42:30 -0500 "J. Bruce Fields" <[email protected]>…
Quoted 6 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 8 lines

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.

Andrew Morton

2012-02-16 22:59 UTC #7 lore raw

Quoted 30 lines · On Thu, 16 Feb 2012 23:37:11 +0100 Davidlohr Bueso <[email protected]> wrote:

Quoted 21 lines · Andrew Morton
Quoted 18 lines · On Wed, 15 Feb 2012 07:42:30 -0500 "J. Bruce Fields" <[email protected]>…
Quoted 6 lines
Quoted 2 lines
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.
Quoted 7 lines
Quoted 2 lines
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!

Quoted 4 lines

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".