[PATCH] fs: remove s_old_blocksize from struct super_block

[PATCH] fs: remove s_old_blocksize from struct super_block

[Pekka Enberg]

Hi,

The s_old_blocksize field of struct super_block is only used as a temporary
variable in get_sb_bdev(). This patch changes the function to use a local
variable instead so we can kill the field from struct super_block.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 fs/super.c         |    5 +++--
 include/linux/fs.h |    1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

Index: 2.6/fs/super.c
===================================================================
--- 2.6.orig/fs/super.c
+++ 2.6/fs/super.c
@@ -707,11 +707,12 @@ struct super_block *get_sb_bdev(struct f
 		goto out;
 	} else {
 		char b[BDEVNAME_SIZE];
+		unsigned long old_blocksize;
 
 		s->s_flags = flags;
 		strlcpy(s->s_id, bdevname(bdev, b), sizeof(s->s_id));
-		s->s_old_blocksize = block_size(bdev);
-		sb_set_blocksize(s, s->s_old_blocksize);
+		old_blocksize = block_size(bdev);
+		sb_set_blocksize(s, old_blocksize);
 		error = fill_super(s, data, flags & MS_VERBOSE ? 1 : 0);
 		if (error) {
 			up_write(&s->s_umount);
Index: 2.6/include/linux/fs.h
===================================================================
--- 2.6.orig/include/linux/fs.h
+++ 2.6/include/linux/fs.h
@@ -777,7 +777,6 @@ struct super_block {
 	struct list_head	s_list;		/* Keep this first */
 	dev_t			s_dev;		/* search index; _not_ kdev_t */
 	unsigned long		s_blocksize;
-	unsigned long		s_old_blocksize;
 	unsigned char		s_blocksize_bits;
 	unsigned char		s_dirt;
 	unsigned long long	s_maxbytes;	/* Max file size */

Re: [PATCH] fs: remove s_old_blocksize from struct super_block

[Pekka Enberg]

Lets kill the variable too as suggested by Joe.

[PATCH] fs: remove s_old_blocksize from struct super_block

This patch inlines the single user of struct super_block field
s_old_blocksize and removes the field.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 fs/super.c         |    3 +--
 include/linux/fs.h |    1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

Index: 2.6/fs/super.c
===================================================================
--- 2.6.orig/fs/super.c
+++ 2.6/fs/super.c
@@ -710,8 +710,7 @@ struct super_block *get_sb_bdev(struct f
 
 		s->s_flags = flags;
 		strlcpy(s->s_id, bdevname(bdev, b), sizeof(s->s_id));
-		s->s_old_blocksize = block_size(bdev);
-		sb_set_blocksize(s, s->s_old_blocksize);
+		sb_set_blocksize(s, block_size(bdev));
 		error = fill_super(s, data, flags & MS_VERBOSE ? 1 : 0);
 		if (error) {
 			up_write(&s->s_umount);
Index: 2.6/include/linux/fs.h
===================================================================
--- 2.6.orig/include/linux/fs.h
+++ 2.6/include/linux/fs.h
@@ -777,7 +777,6 @@ struct super_block {
 	struct list_head	s_list;		/* Keep this first */
 	dev_t			s_dev;		/* search index; _not_ kdev_t */
 	unsigned long		s_blocksize;
-	unsigned long		s_old_blocksize;
 	unsigned char		s_blocksize_bits;
 	unsigned char		s_dirt;
 	unsigned long long	s_maxbytes;	/* Max file size */

Re: [PATCH] fs: remove s_old_blocksize from struct super_block

[Anton Altaparmakov]

Hi,

On Fri, 2 Dec 2005, Pekka Enberg wrote:
> The s_old_blocksize field of struct super_block is only used as a temporary
> variable in get_sb_bdev(). This patch changes the function to use a local
> variable instead so we can kill the field from struct super_block.

s_old_blocksize used to be used to restore the blocksize after the 
filesystem had failed to mount or had unmounted.  Not restoring this leads 
to all sorts of problems since the blocksize may be set for example to 4k 
but some userspace app may need it to be set to 1k or whatever.  There 
used to be applications that failed which is why s_old_blocksize was 
introduced and it used to restore the blocksize.

I have no idea why/when the restoring has been removed but chances are the 
removal was wrong.  Now every file system will need to restore the 
blocksize itself (as it used to be before s_old_blocksize and blocksize 
restoral was introduced).  Except whoever removed the restoration failed 
to fix up all file systems.  )-:

Seems a big step backwards...

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] fs: remove s_old_blocksize from struct super_block

[Pekka Enberg]

Hi Anton,

On Fri, 2 Dec 2005, Pekka Enberg wrote:
> > The s_old_blocksize field of struct super_block is only used as a temporary
> > variable in get_sb_bdev(). This patch changes the function to use a local
> > variable instead so we can kill the field from struct super_block.

On Sat, 2005-12-03 at 11:02 +0000, Anton Altaparmakov wrote:
> s_old_blocksize used to be used to restore the blocksize after the 
> filesystem had failed to mount or had unmounted.  Not restoring this leads 
> to all sorts of problems since the blocksize may be set for example to 4k 
> but some userspace app may need it to be set to 1k or whatever.  There 
> used to be applications that failed which is why s_old_blocksize was 
> introduced and it used to restore the blocksize.
> 
> I have no idea why/when the restoring has been removed but chances are the 
> removal was wrong.  Now every file system will need to restore the 
> blocksize itself (as it used to be before s_old_blocksize and blocksize 
> restoral was introduced).  Except whoever removed the restoration failed 
> to fix up all file systems.  )-:

It was removed in this commit, I think:

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commit;h=294c42046966e927ef86c0d4ce71cff32d9b458c

			Pekka

Re: [PATCH] fs: remove s_old_blocksize from struct super_block

[Anton Altaparmakov]

Hi,

On Sat, 3 Dec 2005, Pekka Enberg wrote:
> On Fri, 2 Dec 2005, Pekka Enberg wrote:
> > > The s_old_blocksize field of struct super_block is only used as a temporary
> > > variable in get_sb_bdev(). This patch changes the function to use a local
> > > variable instead so we can kill the field from struct super_block.
> 
> On Sat, 2005-12-03 at 11:02 +0000, Anton Altaparmakov wrote:
> > s_old_blocksize used to be used to restore the blocksize after the 
> > filesystem had failed to mount or had unmounted.  Not restoring this leads 
> > to all sorts of problems since the blocksize may be set for example to 4k 
> > but some userspace app may need it to be set to 1k or whatever.  There 
> > used to be applications that failed which is why s_old_blocksize was 
> > introduced and it used to restore the blocksize.
> > 
> > I have no idea why/when the restoring has been removed but chances are the 
> > removal was wrong.  Now every file system will need to restore the 
> > blocksize itself (as it used to be before s_old_blocksize and blocksize 
> > restoral was introduced).  Except whoever removed the restoration failed 
> > to fix up all file systems.  )-:
> 
> It was removed in this commit, I think:
> 
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git;a=commit;h=294c42046966e927ef86c0d4ce71cff32d9b458c

Yes, it certainly was removed there.  Thanks for finding the commit.  That 
had gone past me unnoticed.

This means we are now back to the old behaviour where fs utilities will 
behave randomly/unpredictably depending on what fs was mounted (or even 
was tried to be mounted!) on the device last.  So for example a failing 
"mount -t auto" will leave the block size set to a random number when all 
fs utilities (at least used to) asume the block size is 1k and strangeness 
ensues.

I have no idea why Jeff (Mahoney) considered the setting to be 
unnecessary, when Al Viro added the resetting code a few years ago it 
was done precisely because utilities were behaving randomly/erratically...

IMHO the above commit consitutes a regression in 2.6 kernel.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] fs: remove s_old_blocksize from struct super_block

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Anton Altaparmakov wrote:
> I have no idea why Jeff (Mahoney) considered the setting to be 
> unnecessary, when Al Viro added the resetting code a few years ago it 
> was done precisely because utilities were behaving randomly/erratically...
> 
> IMHO the above commit consitutes a regression in 2.6 kernel.

Anton -

Linus and I discussed[1] this briefly on the list in March. Here's my
conclusion[2]. My initial attempts tried to preserve the behavior, but
keeping it at the VFS level made the fix more complex. Linus pointed out
that the file systems explicitly set the block size to a known value
before doing anything. Userspace utilities that don't open the block
device with O_EXCL can have the block size changed underneath them in
mid-execution - by the kernel or another userspace process using the
BLKBSZSET ioctl. Incidentally, the ioctl doesn't save the old value either.

I don't think any userspace utility can expect a particular block size
without an exclusive open and explicit set of the block size.

Pushing the resetting of the block size up into the file system doesn't
eliminate the race. The race exists when get_sb_bdev_excl opens the
block device, which it is allowed to do since it's the same file system
type, and then tries to get the super block. If deactivate_super has
already changed sb->s_count -= S_BIAS -1, sget will ignore the
superblock and allocate a new one. It will then call the file system's
fill_super(). Once a new superblock is allocated, the only thing that
the two have in common is the block device and there is nothing
preventing ->fill_super() from running concurrently with anything under
- ->kill_sb(). This includes ->put_super() as well, so pushing the reset
up into the individual file systems would just preserve/reintroduce the
race.

bdev->bd_mount_sem might be a workable synchronization point for this if
you truly want to preserve the old behavior.

- -Jeff

[1]: http://www.ussg.iu.edu/hypermail/linux/kernel/0503.1/2369.html
[2]: http://www.ussg.iu.edu/hypermail/linux/kernel/0503.2/0460.html

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFDkdkQLPWxlyuTD7IRAo0uAJ4+JQOz5g47rVlGZzwoaELY1G3q1QCfWSi+
HfGC1KAvRhm+YXU47cTK83U=
=JHuY
-----END PGP SIGNATURE-----

Re: [PATCH] fs: remove s_old_blocksize from struct super_block

[Linus Torvalds]

On Sat, 3 Dec 2005, Anton Altaparmakov wrote:
> 
> This means we are now back to the old behaviour where fs utilities will 
> behave randomly/unpredictably depending on what fs was mounted (or even 
> was tried to be mounted!) on the device last.  So for example a failing 
> "mount -t auto" will leave the block size set to a random number when all 
> fs utilities (at least used to) asume the block size is 1k and strangeness 
> ensues.

No, an open() with a zero count should re-set the block size to the "best 
possible one". This gives you _consistently_ the blocksize you should get. 
See bd_set_size().

Which is basically the biggest possible one that we can handle that also 
handles the size of the device correctly (if the device has an odd number 
of sectors, you need to set blocksize to just 512).

So things are consistently "optimal", which is the whole point. An opener 
can then set it to some sub-optimal value (which may be bigger and faster, 
but will mean that the last odd sectors can't be reached) by hand if it 
cares.

And of course, if multiple openers exist, there is no consistent value at 
all. It's a bug in user space (and we don't allow that for mounted block 
devices, since the filesystem often _requires_ a particular blocksize).

> I have no idea why Jeff (Mahoney) considered the setting to be 
> unnecessary, when Al Viro added the resetting code a few years ago it 
> was done precisely because utilities were behaving randomly/erratically...

The old code was buggy. It caused both performance problems (lower 
blocksize than necessary when new openers came in, causing really bad 
throughput) and iirc correctness problems (it would stick a 1kB blocksize 
on a device that couldn't then read the last sector, so accessing the 
device with /dev/xyz wouldn't get all the data).

You can still set the blocksize explicitly, but you need to do it AFTER 
you've opened the device exclusively. 

It was a BUG to do anything else (eg set the blocksize at boot and expect 
that the broken blocksize would get restored after others realized they 
needed something else).

Now, it's entirely possible that we've introduced other breakage since, 
but unless you can show a real case, I think you're barking up the wrong 
tree. 

			Linus