Ralf, please confirm.
[PATCH] ext4: fix #11321: create /proc/ext4/*/stats more carefully
ext4 creates per-suberblock directory in /proc/ext4/ . Name used as
basis is taken from bdevname, which, surprise, can contain slash.
However, proc while allowing to use proc_create("a/b", parent) form of
PDE creation, assumes that parent/a was already created.
bdevname in question is 'cciss/c0d0p9', directory is not created and all
this stuff goes directly into /proc (which is real bug).
Warning comes when _second_ partition is mounted.
http://bugzilla.kernel.org/show_bug.cgi?id=11321
Signed-off-by: Alexey Dobriyan <[email protected]>
---
fs/ext4/mballoc.c | 9 +++++++++
1 file changed, 9 insertions(+)
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2792,6 +2792,15 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
return -EINVAL;
}
bdevname(sb->s_bdev, devname);
+ {
+ char *p = devname;
+
+ while (*p != '\0') {
+ if (*p == '/')
+ *p = '!';
+ p++;
+ }
+ }
sbi->s_mb_proc = proc_mkdir(devname, proc_root_ext4);
MB_PROC_HANDLER(EXT4_MB_STATS_NAME, stats);
On Sep 06, 2008 01:06 +0400, Alexey Dobriyan wrote:
> [PATCH] ext4: fix #11321: create /proc/ext4/*/stats more carefully
>
> ext4 creates per-suberblock directory in /proc/ext4/ . Name used as
> basis is taken from bdevname, which, surprise, can contain slash.
>
> However, proc while allowing to use proc_create("a/b", parent) form of
> PDE creation, assumes that parent/a was already created.
>
> bdevname in question is 'cciss/c0d0p9', directory is not created and all
> this stuff goes directly into /proc (which is real bug).
>
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2792,6 +2792,15 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
> return -EINVAL;
> }
> bdevname(sb->s_bdev, devname);
> + {
> + char *p = devname;
> +
> + while (*p != '\0') {
> + if (*p == '/')
> + *p = '!';
> + p++;
> + }
> + }
Why not use strchr(), which is normally optimized assembly:
char *p = devname;
while ((p = strchr(p, '/'))
*p = '_';
Using '!' as the separator makes it harder to use from shells I suspect,
so I'd suggest '_' instead.
Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.
On Sat, Sep 06, 2008 at 01:57:13AM -0600, Andreas Dilger wrote:
> On Sep 06, 2008 01:06 +0400, Alexey Dobriyan wrote:
> > [PATCH] ext4: fix #11321: create /proc/ext4/*/stats more carefully
> >
> > ext4 creates per-suberblock directory in /proc/ext4/ . Name used as
> > basis is taken from bdevname, which, surprise, can contain slash.
> >
> > However, proc while allowing to use proc_create("a/b", parent) form of
> > PDE creation, assumes that parent/a was already created.
> >
> > bdevname in question is 'cciss/c0d0p9', directory is not created and all
> > this stuff goes directly into /proc (which is real bug).
> >
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -2792,6 +2792,15 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
> > return -EINVAL;
> > }
> > bdevname(sb->s_bdev, devname);
> > + {
> > + char *p = devname;
> > +
> > + while (*p != '\0') {
> > + if (*p == '/')
> > + *p = '!';
> > + p++;
> > + }
> > + }
>
> Why not use strchr(), which is normally optimized assembly:
>
> char *p = devname;
> while ((p = strchr(p, '/'))
> *p = '_';
>
> Using '!' as the separator makes it harder to use from shells I suspect,
> so I'd suggest '_' instead.
bdevname is only 32 bytes and done once per mount, so nobody cares.
'!' is what other code does in this situation (reiserfs, md, ...).
* Alexey Dobriyan <[email protected]>:
> > Why not use strchr(), which is normally optimized assembly:
> >
> > char *p = devname;
> > while ((p = strchr(p, '/'))
> > *p = '_';
> >
> > Using '!' as the separator makes it harder to use from shells I suspect,
> > so I'd suggest '_' instead.
>
> bdevname is only 32 bytes and done once per mount, so nobody cares.
>
> '!' is what other code does in this situation (reiserfs, md, ...).
Anyway, the first patch fixes the issue. Will it go into mainline soon?
--
Ralf Hildebrandt (i.A. des IT-Zentrums) [email protected]
Charite - Universitätsmedizin Berlin Tel. +49 (0)30-450 570-155
Gemeinsame Einrichtung von FU- und HU-Berlin Fax. +49 (0)30-450 570-962
IT-Zentrum Standort CBF I'm looking for a job!
On Sun, Sep 07, 2008 at 04:15:57PM +0400, Alexey Dobriyan wrote:
> > Why not use strchr(), which is normally optimized assembly:
> >
> > char *p = devname;
> > while ((p = strchr(p, '/'))
> > *p = '_';
> >
> > Using '!' as the separator makes it harder to use from shells I suspect,
> > so I'd suggest '_' instead.
>
> bdevname is only 32 bytes and done once per mount, so nobody cares.
Bloat gets inserted into the kernel, 32 bytes at a time. :-)
> '!' is what other code does in this situation (reiserfs, md, ...).
I'm not convinced that the consistency is as important in this case as
making it easy for people using shells typing the pathname...
On Sun, Sep 07, 2008 at 06:04:14PM +0200, Ralf Hildebrandt wrote:
>
> Anyway, the first patch fixes the issue. Will it go into mainline soon?
>
I'll queue the patch (with fixups) for the 2.6.27 merge window. This
isn't a regression, and it's getting rather late in the 2.6.27-rc
series. I've bent the rules about submitting non-regression bug fixes
back when most ext4 users were testers or developers; but now that we
are getting real users, and Linus has started yelling at developers
for ignoring the merge window rules, I'm going to be much more of a
stickler about only pushing regression bug-fixes after the merge
window closes.
- Ted
On Sun, Sep 07, 2008 at 12:24:47PM -0400, Theodore Tso wrote:
> On Sun, Sep 07, 2008 at 04:15:57PM +0400, Alexey Dobriyan wrote:
> > > Why not use strchr(), which is normally optimized assembly:
> > >
> > > char *p = devname;
> > > while ((p = strchr(p, '/'))
> > > *p = '_';
> > >
> > > Using '!' as the separator makes it harder to use from shells I suspect,
> > > so I'd suggest '_' instead.
> >
> > bdevname is only 32 bytes and done once per mount, so nobody cares.
>
> Bloat gets inserted into the kernel, 32 bytes at a time. :-)
You mean, one filesystem at time. ;-)
> > '!' is what other code does in this situation (reiserfs, md, ...).
>
> I'm not convinced that the consistency is as important in this case as
> making it easy for people using shells typing the pathname...
A what?
mkdir z\!ext4
ls z[Tab]\!ext4
As for underscore, use '-' if you really care about typing.
One Shift less.
register_disk() uses '!' too, BTW.
On Sun, Sep 07, 2008 at 08:41:30PM +0400, Alexey Dobriyan wrote:
>
> register_disk() uses '!' too, BTW.
>
Yeah, I guess it's good to be consistent with what's being broadcast
via udev et al. I still that '!' in pathnames or '/' in device names
is really gross and shows no taste, though. :-)
Here's what I've checked into the ext4 patch queue for submission to
mainline at the next merge window. I've added a bit more error
checking in case proc_mkdir() fails and returns NULL.
- Ted
ext4: fix #11321: create /proc/ext4/*/stats more carefully
From: Alexey Dobriyan <[email protected]>
ext4 creates per-suberblock directory in /proc/ext4/ . Name used as
basis is taken from bdevname, which, surprise, can contain slash.
However, proc while allowing to use proc_create("a/b", parent) form of
PDE creation, assumes that parent/a was already created.
bdevname in question is 'cciss/c0d0p9', directory is not created and all
this stuff goes directly into /proc (which is real bug).
Warning comes when _second_ partition is mounted.
http://bugzilla.kernel.org/show_bug.cgi?id=11321
Signed-off-by: Alexey Dobriyan <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2721643..984144d 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2785,14 +2785,19 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
mode_t mode = S_IFREG | S_IRUGO | S_IWUSR;
struct ext4_sb_info *sbi = EXT4_SB(sb);
struct proc_dir_entry *proc;
- char devname[64];
+ char devname[64], *p;
if (proc_root_ext4 == NULL) {
sbi->s_mb_proc = NULL;
return -EINVAL;
}
bdevname(sb->s_bdev, devname);
+ while (p = strchr(p, '/'))
+ *p = '!';
+
sbi->s_mb_proc = proc_mkdir(devname, proc_root_ext4);
+ if (!sbi->s_mb_proc)
+ goto err_create_dir;
MB_PROC_HANDLER(EXT4_MB_STATS_NAME, stats);
MB_PROC_HANDLER(EXT4_MB_MAX_TO_SCAN_NAME, max_to_scan);
@@ -2804,7 +2809,6 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
return 0;
err_out:
- printk(KERN_ERR "EXT4-fs: Unable to create %s\n", devname);
remove_proc_entry(EXT4_MB_GROUP_PREALLOC, sbi->s_mb_proc);
remove_proc_entry(EXT4_MB_STREAM_REQ, sbi->s_mb_proc);
remove_proc_entry(EXT4_MB_ORDER2_REQ, sbi->s_mb_proc);
@@ -2813,6 +2817,8 @@ err_out:
remove_proc_entry(EXT4_MB_STATS_NAME, sbi->s_mb_proc);
remove_proc_entry(devname, proc_root_ext4);
sbi->s_mb_proc = NULL;
+err_create_dir:
+ printk(KERN_ERR "EXT4-fs: Unable to create %s\n", devname);
return -ENOMEM;
}
On Mon, Sep 08, 2008 at 10:39:51AM -0400, Theodore Tso wrote:
> Here's what I've checked into the ext4 patch queue for submission to
> mainline at the next merge window. I've added a bit more error
> checking in case proc_mkdir() fails and returns NULL.
Hopefully, Andrew, will pick up original non-broken patch.
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2785,14 +2785,19 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
> mode_t mode = S_IFREG | S_IRUGO | S_IWUSR;
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> struct proc_dir_entry *proc;
> - char devname[64];
> + char devname[64], *p;
>
> if (proc_root_ext4 == NULL) {
> sbi->s_mb_proc = NULL;
> return -EINVAL;
> }
> bdevname(sb->s_bdev, devname);
> + while (p = strchr(p, '/'))
> + *p = '!';
> +
> sbi->s_mb_proc = proc_mkdir(devname, proc_root_ext4);
> + if (!sbi->s_mb_proc)
> + goto err_create_dir;
>
> MB_PROC_HANDLER(EXT4_MB_STATS_NAME, stats);
> MB_PROC_HANDLER(EXT4_MB_MAX_TO_SCAN_NAME, max_to_scan);
> @@ -2804,7 +2809,6 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
> return 0;
>
> err_out:
> - printk(KERN_ERR "EXT4-fs: Unable to create %s\n", devname);
> remove_proc_entry(EXT4_MB_GROUP_PREALLOC, sbi->s_mb_proc);
> remove_proc_entry(EXT4_MB_STREAM_REQ, sbi->s_mb_proc);
> remove_proc_entry(EXT4_MB_ORDER2_REQ, sbi->s_mb_proc);
> @@ -2813,6 +2817,8 @@ err_out:
> remove_proc_entry(EXT4_MB_STATS_NAME, sbi->s_mb_proc);
> remove_proc_entry(devname, proc_root_ext4);
> sbi->s_mb_proc = NULL;
> +err_create_dir:
> + printk(KERN_ERR "EXT4-fs: Unable to create %s\n", devname);
>
> return -ENOMEM;
> }
On Tue, 9 Sep 2008 11:06:30 +0400 Alexey Dobriyan <[email protected]> wrote:
> On Mon, Sep 08, 2008 at 10:39:51AM -0400, Theodore Tso wrote:
> > Here's what I've checked into the ext4 patch queue for submission to
> > mainline at the next merge window. I've added a bit more error
> > checking in case proc_mkdir() fails and returns NULL.
>
> Hopefully, Andrew, will pick up original non-broken patch.
What does this mean??
On Mon, Sep 08, 2008 at 10:39:51AM -0400, Theodore Tso wrote:
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2785,14 +2785,19 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
> mode_t mode = S_IFREG | S_IRUGO | S_IWUSR;
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> struct proc_dir_entry *proc;
> - char devname[64];
> + char devname[64], *p;
>
> if (proc_root_ext4 == NULL) {
> sbi->s_mb_proc = NULL;
> return -EINVAL;
> }
> bdevname(sb->s_bdev, devname);
> + while (p = strchr(p, '/'))
> + *p = '!';
This can't work because "p" is uninitialized. And gcc whines about one
more pair of ().
On Sat, Sep 06, 2008 at 01:06:52AM +0400, Alexey Dobriyan wrote:
> Ralf, please confirm.
>
>
>
> [PATCH] ext4: fix #11321: create /proc/ext4/*/stats more carefully
>
> ext4 creates per-suberblock directory in /proc/ext4/ . Name used as
> basis is taken from bdevname, which, surprise, can contain slash.
>
> However, proc while allowing to use proc_create("a/b", parent) form of
> PDE creation, assumes that parent/a was already created.
>
> bdevname in question is 'cciss/c0d0p9', directory is not created and all
> this stuff goes directly into /proc (which is real bug).
>
> Warning comes when _second_ partition is mounted.
Two issues with this. First filesystems should never call
bdevname(sb->s_bdev, ...), but alwasy us s->s_id which has this
pre-calculated and avoids the big on-stack array. Second this
conversion really isn't extN specific, so I'd rather do it in
get_sb_bdev so one places takes care of all uses, instead of
re-inventing it in lots of places.
On Tue, Sep 09, 2008 at 12:12:03AM -0700, Andrew Morton wrote:
> On Tue, 9 Sep 2008 11:06:30 +0400 Alexey Dobriyan <[email protected]> wrote:
>
> > On Mon, Sep 08, 2008 at 10:39:51AM -0400, Theodore Tso wrote:
> > > Here's what I've checked into the ext4 patch queue for submission to
> > > mainline at the next merge window. I've added a bit more error
> > > checking in case proc_mkdir() fails and returns NULL.
> >
> > Hopefully, Andrew, will pick up original non-broken patch.
>
> What does this mean??
Alexey's trying to bypass the ext4 maintainers. :-)
Alexey, both of the problems which you pointed out I noticed last
night and have already been fixed and commited to the ext4 patch
queue. Please see attached. This patch *is* better than your
original one since using strdup is better than open coding it in C.
Andrew, note that some of the patches in the upcoming ext4 patch set
which I am preparing for -mm and linux-next submission depend on a the
percpu cleanup patch which is in already in -mm. I've left it in the
ext4 series file since the patchset won't apply against mainline
without it, but I've left comments in the series file that should make
this clear.
I'm not sure how you are managing -mm these days, but it looks like
you are using git more, and so if you are pulling in changes in via
linux-next, git will do the right thing and drop the duplicated patch.
There is a similar issue with the FIEMAP patches and some ext3
patches, but the FIEMAP patches will get dropped since Eric has
promised to get on Mark Fasheh's case to submit for merging or to
submit them yourself, and the ext3 patches I will submit to you
separately.
Regards,
- Ted
ext4: fix #11321: create /proc/ext4/*/stats more carefully
From: Alexey Dobriyan <[email protected]>
ext4 creates per-suberblock directory in /proc/ext4/ . Name used as
basis is taken from bdevname, which, surprise, can contain slash.
However, proc while allowing to use proc_create("a/b", parent) form of
PDE creation, assumes that parent/a was already created.
bdevname in question is 'cciss/c0d0p9', directory is not created and all
this stuff goes directly into /proc (which is real bug).
Warning comes when _second_ partition is mounted.
http://bugzilla.kernel.org/show_bug.cgi?id=11321
Signed-off-by: Alexey Dobriyan <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 2721643..78d628b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2785,14 +2785,20 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
mode_t mode = S_IFREG | S_IRUGO | S_IWUSR;
struct ext4_sb_info *sbi = EXT4_SB(sb);
struct proc_dir_entry *proc;
- char devname[64];
+ char devname[64], *p;
if (proc_root_ext4 == NULL) {
sbi->s_mb_proc = NULL;
return -EINVAL;
}
bdevname(sb->s_bdev, devname);
+ p = devname;
+ while ((p = strchr(p, '/')))
+ *p = '!';
+
sbi->s_mb_proc = proc_mkdir(devname, proc_root_ext4);
+ if (!sbi->s_mb_proc)
+ goto err_create_dir;
MB_PROC_HANDLER(EXT4_MB_STATS_NAME, stats);
MB_PROC_HANDLER(EXT4_MB_MAX_TO_SCAN_NAME, max_to_scan);
@@ -2804,7 +2810,6 @@ static int ext4_mb_init_per_dev_proc(struct super_block *sb)
return 0;
err_out:
- printk(KERN_ERR "EXT4-fs: Unable to create %s\n", devname);
remove_proc_entry(EXT4_MB_GROUP_PREALLOC, sbi->s_mb_proc);
remove_proc_entry(EXT4_MB_STREAM_REQ, sbi->s_mb_proc);
remove_proc_entry(EXT4_MB_ORDER2_REQ, sbi->s_mb_proc);
@@ -2813,6 +2818,8 @@ err_out:
remove_proc_entry(EXT4_MB_STATS_NAME, sbi->s_mb_proc);
remove_proc_entry(devname, proc_root_ext4);
sbi->s_mb_proc = NULL;
+err_create_dir:
+ printk(KERN_ERR "EXT4-fs: Unable to create %s\n", devname);
return -ENOMEM;
}
On Tue, 9 Sep 2008 09:09:00 -0400 Theodore Tso <[email protected]> wrote:
> On Tue, Sep 09, 2008 at 12:12:03AM -0700, Andrew Morton wrote:
> > On Tue, 9 Sep 2008 11:06:30 +0400 Alexey Dobriyan <[email protected]> wrote:
> >
> > > On Mon, Sep 08, 2008 at 10:39:51AM -0400, Theodore Tso wrote:
> > > > Here's what I've checked into the ext4 patch queue for submission to
> > > > mainline at the next merge window. I've added a bit more error
> > > > checking in case proc_mkdir() fails and returns NULL.
> > >
> > > Hopefully, Andrew, will pick up original non-broken patch.
> >
> > What does this mean??
>
> Alexey's trying to bypass the ext4 maintainers. :-)
>
> Alexey, both of the problems which you pointed out I noticed last
> night and have already been fixed and commited to the ext4 patch
> queue. Please see attached. This patch *is* better than your
> original one since using strdup is better than open coding it in C.
>
> Andrew, note that some of the patches in the upcoming ext4 patch set
> which I am preparing for -mm and linux-next submission depend on a the
> percpu cleanup patch which is in already in -mm.
"the percpu cleanup patch" is nowhere nearly specific enough to be
useful. I don't know what patch this is.
> I've left it in the
> ext4 series file since the patchset won't apply against mainline
> without it, but I've left comments in the series file that should make
> this clear.
>
> I'm not sure how you are managing -mm these days, but it looks like
> you are using git more, and so if you are pulling in changes in via
> linux-next, git will do the right thing and drop the duplicated patch.
> There is a similar issue with the FIEMAP patches and some ext3
> patches, but the FIEMAP patches will get dropped since Eric has
> promised to get on Mark Fasheh's case to submit for merging or to
> submit them yourself, and the ext3 patches I will submit to you
> separately.
>
If stuff turns up in linux-next then I'll just drop the -mm duplicate
under the assumption that the patch is being taken care of by someone
else. I will usually attempt to verify that the subsystem tree merged
the correct patch. Fairly often they didn't. Fairly often this is
because they took the original patch off the mailing list and omitted
fixes which I (or others) had made to it. This is why I keep those
fixes as separate patches until the last minute.
If other patches in -mm have dependencies upon that patch then things
can sometimes get sticky, but as long as I know about it I can usually
get things in the right order.
On Tue, Sep 09, 2008 at 11:10:06AM -0700, Andrew Morton wrote:
>
> "the percpu cleanup patch" is nowhere nearly specific enough to be
> useful. I don't know what patch this is.
Sorry, I should have been more specific. It's called:
percpu-counters-clean-up-percpu_counter_sum_and_set-interface.patch
in in your mmotm tree.
> If stuff turns up in linux-next then I'll just drop the -mm duplicate
> under the assumption that the patch is being taken care of by someone
> else.
Well, in the past you've been asked me not to include non-ext4 patches
in the ext4 tree. So I could include that patch in an ext4 patchset
and push it to linux-next, since it *does* involve changes to fs/ext4
as well as to include/linux/percpu_counter.h and lib/percpu_counter.c,
if that would be easier. Or we can keep it out of the set of patches
I push to Linus, but then we have to worry about patch ordering and
dependencies a bit more.
> I will usually attempt to verify that the subsystem tree merged
> the correct patch. Fairly often they didn't.
I've checked, and modulo some minor changelog comment fixups I had
made to fix grammar and spelling that I had made to my version of the
patch, and the fact that we didn't have your signed-off-by in your
version of the patch, the patch we have is identical. I'll reconcile
the changelog comments headers on my end, and if you want to keep it
in -mm, I'll send them back to you. Otherwise I'll include it in ext4
patches and you can drop it from yours if that's OK with you.
Whatever's easier....
- Ted
On Tue, 9 Sep 2008 14:24:57 -0400 Theodore Tso <[email protected]> wrote:
> > I will usually attempt to verify that the subsystem tree merged
> > the correct patch. Fairly often they didn't.
>
> I've checked, and modulo some minor changelog comment fixups I had
> made to fix grammar and spelling that I had made to my version of the
> patch, and the fact that we didn't have your signed-off-by in your
> version of the patch, the patch we have is identical. I'll reconcile
> the changelog comments headers on my end, and if you want to keep it
> in -mm, I'll send them back to you. Otherwise I'll include it in ext4
> patches and you can drop it from yours if that's OK with you.
> Whatever's easier....
Nope, sounds good - please go ahead and merge it up.