fs: remove s_old_blocksize from struct super_block

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.

---

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

Index: 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
===================================================================
@@ -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 */

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.

---

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

Index: 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
===================================================================
@@ -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 */

Hi,

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/

Hi Anton,

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

Hi,

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/

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

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

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

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