JFS resize=0 problem in 2.6.0

JFS resize=0 problem in 2.6.0

[lkml]

Let me know if I'm missing the goal of the code here, but lines 261-273
of linux-2.6.0/fs/jfs/super.c are:

case Opt_resize:
{
	char *resize = args[0].from;
	if (!resize || !*resize) {    /* LINE 264 HERE */
		*newLVSize = sb->s_bdev->bd_inode->i_size >>
			sb->s_blocksize_bits;
		if (*newLVSize == 0)
			printk(KERN_ERR
			"JFS: Cannot determine volume size\n");
	} else
		*newLVSize = simple_strtoull(resize, &resize, 0);
	break;
}

It seems to me that line 264 is attempting to test for the mount 
paramater "resize=0", and when it comes across this, resize to the full
size of the volume.  However, this doesn't work.  I believe it should
test for the char '0'  (*resize=='0'), not against literal zero.  

Let me know if I'm way off base here.  But the below patch does allow a
$ mount -o remount,resize=0 /mnt/test    
to resize the jfs filesystem to the full size of the volume.


Ok, I've never submitted a patch before, but here goes:



--- linux-2.6.0/fs/jfs/super.c  2003-12-28 10:16:00.000000000 -0500
+++ linux/fs/jfs/super.c        2003-12-28 10:15:34.000000000 -0500
@@ -261,7 +261,7 @@
		case Opt_resize:
		{
			char *resize = args[0].from;
-			if (!resize || !*resize) {
+			if (!resize || !*resize || *resize=='0') {
				*newLVSize = sb->s_bdev->bd_inode->i_size >>
					sb->s_blocksize_bits;
				if (*newLVSize == 0)



-Elliott Bennett
lkml@dhtns.com

Re: JFS resize=0 problem in 2.6.0

[Mike Fedyk]

On Sun, Dec 28, 2003 at 10:30:28AM -0500, lkml@dhtns.com wrote:
> It seems to me that line 264 is attempting to test for the mount 
> paramater "resize=0", and when it comes across this, resize to the full
> size of the volume.  However, this doesn't work.  I believe it should
> test for the char '0'  (*resize=='0'), not against literal zero.  
> 
> Let me know if I'm way off base here.  But the below patch does allow a
> $ mount -o remount,resize=0 /mnt/test    
> to resize the jfs filesystem to the full size of the volume.

And it won't without the patch?

What errors do you get if it fails without the patch?

Re: JFS resize=0 problem in 2.6.0

[Elliott Bennett]

On Sun, Dec 28, 2003 at 04:05:03PM -0800, Mike Fedyk wrote:
> On Sun, Dec 28, 2003 at 10:30:28AM -0500, lkml@dhtns.com wrote:
> > It seems to me that line 264 is attempting to test for the mount 
> > paramater "resize=0", and when it comes across this, resize to the full
> > size of the volume.  However, this doesn't work.  I believe it should
> > test for the char '0'  (*resize=='0'), not against literal zero.  
> > 
> > Let me know if I'm way off base here.  But the below patch does allow a
> > $ mount -o remount,resize=0 /mnt/test    
> > to resize the jfs filesystem to the full size of the volume.
> 
> And it won't without the patch?
> 
> What errors do you get if it fails without the patch?

It won't without the patch.  

No errors...it just doesn't resize.  :)

Here's a shell snippit.  I have just resized /dev/vg/lv from 700M to
900M, and I'm running the ORIGINAL jfs module:

root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv       713500       220    713280   1% /mnt
root@tesla:~# mount
/dev/mapper/vg-lv on /mnt type jfs (rw)
root@tesla:~# mount -o remount,resize=0 /mnt
root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv       713500       220    713280   1% /mnt

	..the resizing failed.  But, switching to the patched module:

root@tesla:~# umount /mnt
root@tesla:~# rmmod jfs
root@tesla:~# cp ~/jfs_patch.ko /lib/modules/2.6.0/kernel/fs/jfs/jfs.ko
root@tesla:~# mount -t jfs /dev/vg/lv /mnt
root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv       713500       220    713280   1% /mnt
root@tesla:~# mount -o remount,resize=0 /mnt
root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv       917272       244    917028   1% /mnt

	resizing works.

	
	Oddly, now, an additional lvextend and remount/resize fails:

root@tesla:~# lvextend /dev/vg/lv -L +200M
  /dev/hdd: open failed: Read-only file system
  Extending logical volume lv to 1.07 GB
  Logical volume lv successfully resized
root@tesla:~# mount -o remount,resize=0 /mnt
root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv       917272       244    917028   1% /mnt
root@tesla:~# mount
/dev/mapper/vg-lv on /mnt type jfs (rw,resize=0,resize=0)

	note "resize=0,resize=0" above.
	...but unmounting it, remounting it, and then resizing it works:

root@tesla:~# umount /mnt
root@tesla:~# mount -t jfs /dev/vg/lv /mnt
root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv       917272       244    917028   1% /mnt
root@tesla:~# mount -o remount,resize=0 /mnt
root@tesla:~# df
Filesystem           1K-blocks      Used Available Use% Mounted on
/dev/mapper/vg-lv      1121040       272   1120768   1% /mnt
root@tesla:~# mount
/dev/mapper/vg-lv on /mnt type jfs (rw,resize=0)

	It turns out that it's not so much of the fact that this is a
"second" extention during the same mount as the fact that it was
extended while mounted.  It seems that nomatter when I lvextend (mounted
or unmounted), i must unmount (if mounted), then mount, then
remount/resize for a resize to take effect.  If I don't unmount after
lvextending, and try to resize, I get:
jfs_extendfs: volume hasn't grown, returning


Soo...I would say that something is awry with the resizing of JFS
filesystems.  My patch obviously doesn't fix everything, but at least
makes resizing to fill available space *possible*. :)

The code surrounding my patch change treats the same variable (resize,
which is a pointer to args[0].from) as a string, so it seems pretty
obvious to me it should be comparing to '0'.


-Elliott Bennett

Re: JFS resize=0 problem in 2.6.0

[Mike Fedyk]

On Fri, Jan 02, 2004 at 03:12:21PM -0500, Elliott Bennett wrote:
> Soo...I would say that something is awry with the resizing of JFS
> filesystems.  My patch obviously doesn't fix everything, but at least
> makes resizing to fill available space *possible*. :)
> 
> The code surrounding my patch change treats the same variable (resize,
> which is a pointer to args[0].from) as a string, so it seems pretty
> obvious to me it should be comparing to '0'.

I would be careful of DM and MD RAID in 2.6.0.  There are some bugs flying
around mentioning XFS->DM->MD RAID, , but also reproducable with Ext3->DM.

So if you're using DM, you might want to do some extra consistancy checks in
your tests, and don't use it with important data.

Re: JFS resize=0 problem in 2.6.0

[Christophe Saout]

Am Fr, den 02.01.2004 schrieb Mike Fedyk um 22:24:

> I would be careful of DM and MD RAID in 2.6.0.  There are some bugs flying
> around mentioning XFS->DM->MD RAID, , but also reproducable with Ext3->DM.
> 
> So if you're using DM, you might want to do some extra consistancy checks in
> your tests, and don't use it with important data.

Only DM or DM->RAID? There is one bug on bugzilla mentioning XFS
problems on a >2TB LVM volume dating back to 2.5.73. DM uses sector_t
everywhere, so perhaps it might be a different problem.

DM has a workaround included since one of the later test kernels so that
it should not break on top of the raid code. Also someone posted
bugfixes not too long ago that fixed some hard to trigger bugs with
bi_idx not always starting at zero (which should have affected raid and
dm code under rare circumstances). These bugfixes are in 2.6.1-pre1 I
think.

There is also one bug that isn't fixed in 2.6.0 (but in the -mm kernels
for some time and also in 2.6.1-pre1), it concerns online resizing of
mounted filesystems.

At least I think the core DM code should be safe under normal usage. I
bombed it with all kinds of BIOs, not only the page sized ones most
filesystems and swap code create, using an IDE disk. I'm using it on all
my systems for a long time now.

Elliot's problem is a different though, the jfs option parser doesn't do
what he wants. ;)

Re: JFS resize=0 problem in 2.6.0

[Christophe Saout]

Am So, den 28.12.2003 schrieb lkml@dhtns.com um 16:30:

> Let me know if I'm missing the goal of the code here, but lines 261-273
> of linux-2.6.0/fs/jfs/super.c are:
> 
> case Opt_resize:
> {
> 	char *resize = args[0].from;
> 	if (!resize || !*resize) {    /* LINE 264 HERE */
> 		*newLVSize = sb->s_bdev->bd_inode->i_size >>
> 			sb->s_blocksize_bits;
> 		if (*newLVSize == 0)
> 			printk(KERN_ERR
> 			"JFS: Cannot determine volume size\n");
> 	} else
> 		*newLVSize = simple_strtoull(resize, &resize, 0);
> 	break;
> }
> 
> It seems to me that line 264 is attempting to test for the mount 
> paramater "resize=0", and when it comes across this, resize to the full
> size of the volume.  However, this doesn't work.  I believe it should
> test for the char '0'  (*resize=='0'), not against literal zero.  

literal zero is the end of the string.

So the code checks for resize= and not resize=0.

I think your fix is wrong because it would also recognize resize=0123
because it only tests the first character.

-           if (!resize || !*resize) {
+           if (!resize || !*resize || *resize=='0')

It should probably be

+           if (!resize || !*resize || (*resize=='0' && !resize[1]))

Or better: check the integer value that is returned by simple_strtoull.

But did you test what resize= does?

Re: JFS resize=0 problem in 2.6.0

[Elliott Bennett]

On Fri, Jan 02, 2004 at 11:33:03PM +0100, Christophe Saout wrote:
> Am So, den 28.12.2003 schrieb lkml@dhtns.com um 16:30:
> 
> > Let me know if I'm missing the goal of the code here, but lines 261-273
> > of linux-2.6.0/fs/jfs/super.c are:
> > 
> > case Opt_resize:
> > {
> > 	char *resize = args[0].from;
> > 	if (!resize || !*resize) {    /* LINE 264 HERE */
> > 		*newLVSize = sb->s_bdev->bd_inode->i_size >>
> > 			sb->s_blocksize_bits;
> > 		if (*newLVSize == 0)
> > 			printk(KERN_ERR
> > 			"JFS: Cannot determine volume size\n");
> > 	} else
> > 		*newLVSize = simple_strtoull(resize, &resize, 0);
> > 	break;
> > }
> > 
> > It seems to me that line 264 is attempting to test for the mount 
> > paramater "resize=0", and when it comes across this, resize to the full
> > size of the volume.  However, this doesn't work.  I believe it should
> > test for the char '0'  (*resize=='0'), not against literal zero.  
> 
> literal zero is the end of the string.
> 
> So the code checks for resize= and not resize=0.
> 
> I think your fix is wrong because it would also recognize resize=0123
> because it only tests the first character.
> 
> -           if (!resize || !*resize) {
> +           if (!resize || !*resize || *resize=='0')
> 
> It should probably be
> 
> +           if (!resize || !*resize || (*resize=='0' && !resize[1]))
> 
> Or better: check the integer value that is returned by simple_strtoull.
> 
> But did you test what resize= does?
> 
> 

Good catch!  I'm embarrassed that I didn't see that.  

As for "resize=", it seems to fail.  Probably because it doesn't match
the "resize=%u" pattern:

root@tesla:~# mount -o remount,resize= /mnt
mount: /mnt not mounted already, or bad option

Someone posted a patch the other day that supposedly handles both
"resize=0" and "resize" alone..I believe by simply adding it to the
pattern list.

-Elliott

Re: JFS resize=0 problem in 2.6.0

[Dave Kleikamp]

Sorry for not replying sooner.  I was on vacation.

On Sun, 2003-12-28 at 09:30, lkml@dhtns.com wrote:
> Let me know if I'm missing the goal of the code here, but lines 261-273
> of linux-2.6.0/fs/jfs/super.c are:
> 
> case Opt_resize:
> {
> 	char *resize = args[0].from;
> 	if (!resize || !*resize) {    /* LINE 264 HERE */
> 		*newLVSize = sb->s_bdev->bd_inode->i_size >>
> 			sb->s_blocksize_bits;
> 		if (*newLVSize == 0)
> 			printk(KERN_ERR
> 			"JFS: Cannot determine volume size\n");
> 	} else
> 		*newLVSize = simple_strtoull(resize, &resize, 0);
> 	break;
> }
> 
> It seems to me that line 264 is attempting to test for the mount 
> paramater "resize=0", and when it comes across this, resize to the full
> size of the volume.  However, this doesn't work.  I believe it should
> test for the char '0'  (*resize=='0'), not against literal zero.  

Originally, the code accepted "resize" or "resize=" as an indication
that it should resize to the block device size.  Recently, the parsing
code was cleaned up to use the match_token() function, and "resize"
without an argument is no longer accepted.

The following patch should allow the resize option without an argument,
as well as allowing resize=0 to indicate the same thing.

diff -ur linux-2.6.0/fs/jfs/super.c linux/fs/jfs/super.c
--- linux-2.6.0/fs/jfs/super.c	2004-01-05 10:54:19.000000000 -0600
+++ linux/fs/jfs/super.c	2004-01-05 10:54:36.000000000 -0600
@@ -203,7 +203,7 @@
 
 enum {
 	Opt_integrity, Opt_nointegrity, Opt_iocharset, Opt_resize,
-	Opt_errors, Opt_ignore, Opt_err,
+	Opt_resize_nosize, Opt_errors, Opt_ignore, Opt_err,
 };
 
 static match_table_t tokens = {
@@ -211,6 +211,7 @@
 	{Opt_nointegrity, "nointegrity"},
 	{Opt_iocharset, "iocharset=%s"},
 	{Opt_resize, "resize=%u"},
+	{Opt_resize_nosize, "resize"},
 	{Opt_errors, "errors=%s"},
 	{Opt_ignore, "noquota"},
 	{Opt_ignore, "quota"},
@@ -261,7 +262,7 @@
 		case Opt_resize:
 		{
 			char *resize = args[0].from;
-			if (!resize || !*resize) {
+			if (!resize || !*resize || resize == 0) {
 				*newLVSize = sb->s_bdev->bd_inode->i_size >>
 					sb->s_blocksize_bits;
 				if (*newLVSize == 0)
@@ -271,6 +272,15 @@
 				*newLVSize = simple_strtoull(resize, &resize, 0);
 			break;
 		}
+		case Opt_resize_nosize:
+		{
+			*newLVSize = sb->s_bdev->bd_inode->i_size >>
+				sb->s_blocksize_bits;
+			if (*newLVSize == 0)
+				printk(KERN_ERR
+				       "JFS: Cannot determine volume size\n");
+			break;
+		}
 		case Opt_errors:
 		{
 			char *errors = args[0].from;

Thanks,
Shaggy
-- 
David Kleikamp
IBM Linux Technology Center