FAT statfs loop abort on read-error

FAT statfs loop abort on read-error

[Sancho Dauskardt]

Hi all,

  i've written to the current FAT maintainer (Gordon Chaffee) about this, 
but he's no longer active, so:

While working in the usb-sotrage area (mostly with removeable media, eg. 
CompactFlash in USB-Readers), i've come across a litte odd behaviour:

  when calling statfs on a volume that has been removed (without umount) 
fat_statfs() will attempt to read all sectors of the fat table quite a few 
times (depending on the fat type, eg. FAT16 --> 256 times).

eg:
1. mount /dev/sda1 /mnt/cf
2. remove card
3. df

on my system, for a 16 MB CompactFlash formated with FAT-16 this takes 47 
seconds.

Possible solution:
1. let default_fat_access return something like -2 on 'can't read' error.
2. Abort stafs loop on error.
3. return -EIO

This would break mode fat_access calls. I could make a patch, but I don't 
know what's going on with those cvf extensions (which seem to replace 
fat_access). Is dmsdos dead / can we ignore it ?
Somewhere in the list archives, I found comments about the cvf stuff being 
completely removed ?


Thanks,
- sda

Re: FAT statfs loop abort on read-error

[Randy.Dunlap]

On Fri, 04 Jul 2003 13:57:19 +0200 Sancho Dauskardt <sda@bdit.de> wrote:

| Hi all,
| 
|   i've written to the current FAT maintainer (Gordon Chaffee) about this, 
| but he's no longer active, so:
| 
| While working in the usb-sotrage area (mostly with removeable media, eg. 
| CompactFlash in USB-Readers), i've come across a litte odd behaviour:
| 
|   when calling statfs on a volume that has been removed (without umount) 
| fat_statfs() will attempt to read all sectors of the fat table quite a few 
| times (depending on the fat type, eg. FAT16 --> 256 times).
| 
| eg:
| 1. mount /dev/sda1 /mnt/cf
| 2. remove card
| 3. df
| 
| on my system, for a 16 MB CompactFlash formated with FAT-16 this takes 47 
| seconds.
| 
| Possible solution:
| 1. let default_fat_access return something like -2 on 'can't read' error.
| 2. Abort stafs loop on error.
| 3. return -EIO
| 
| This would break mode fat_access calls. I could make a patch, but I don't 
| know what's going on with those cvf extensions (which seem to replace 
| fat_access). Is dmsdos dead / can we ignore it ?
| Somewhere in the list archives, I found comments about the cvf stuff being 
| completely removed ?

Try asking OGAWA Hirofumi (cc-ed).  He's the de facto FAT maintainter.
(I asked him to add a patch to MAINTAINTERS...)

--
~Randy
| http://developer.osdl.org/rddunlap/ | http://www.xenotime.net/linux/ |

Re: FAT statfs loop abort on read-error

[OGAWA Hirofumi]

"Randy.Dunlap" <rddunlap@osdl.org> writes:

> On Fri, 04 Jul 2003 13:57:19 +0200 Sancho Dauskardt <sda@bdit.de> wrote:
> |   when calling statfs on a volume that has been removed (without umount) 
> | fat_statfs() will attempt to read all sectors of the fat table quite a few 
> | times (depending on the fat type, eg. FAT16 --> 256 times).

Yes, fat driver of 2.4 ignore the many errors.

> | Possible solution:
> | 1. let default_fat_access return something like -2 on 'can't read' error.
> | 2. Abort stafs loop on error.
> | 3. return -EIO
> | 
> | This would break mode fat_access calls. I could make a patch, but I don't 
> | know what's going on with those cvf extensions (which seem to replace 
> | fat_access). Is dmsdos dead / can we ignore it ?
> | Somewhere in the list archives, I found comments about the cvf stuff being 
> | completely removed ?

I don't know anybody ported dmsdos to 2.4. The cvf stuff was removed
and many error handlings was fixed on 2.5.x. So, personally I think to
remove the cvf stuff and backport the some parts of fat driver to 2.4
is good.

> (I asked him to add a patch to MAINTAINTERS...)

Thank you. But honestly, I may not have skill enough.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: FAT statfs loop abort on read-error

[Christoph Hellwig]

On Tue, Jul 08, 2003 at 12:54:48AM +0900, OGAWA Hirofumi wrote:
> > (I asked him to add a patch to MAINTAINTERS...)
> 
> Thank you. But honestly, I may not have skill enough.

Given that you have done a nice job ob fatfs in 2.5 and there's no one
coming near that many useful contributions in that timeframe I think
it would be a good idea to declare you maintainer.  According to
MAINTAINERS it currently doesn't have any formal maintainer anyway.

Re: FAT statfs loop abort on read-error

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jul 08, 2003 at 12:54:48AM +0900, OGAWA Hirofumi wrote:
> > > (I asked him to add a patch to MAINTAINTERS...)
> > 
> > Thank you. But honestly, I may not have skill enough.
> 
> Given that you have done a nice job ob fatfs in 2.5 and there's no one
> coming near that many useful contributions in that timeframe I think
> it would be a good idea to declare you maintainer.  According to
> MAINTAINERS it currently doesn't have any formal maintainer anyway.

Thank you very much. I try it.

Please apply the following patch.

 MAINTAINERS |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff -puN MAINTAINERS~maintainers MAINTAINERS
--- linux-2.5.74/MAINTAINERS~maintainers	2003-07-08 20:58:35.000000000 +0900
+++ linux-2.5.74-hirofumi/MAINTAINERS	2003-07-08 21:00:18.000000000 +0900
@@ -2089,10 +2089,9 @@ W:	http://user-mode-linux.sourceforge.ne
 S:	Maintained
 	
 VFAT FILESYSTEM:
-P:	Gordon Chaffee
-M:	chaffee@cs.berkeley.edu
+P:	OGAWA Hirofumi
+M:	hirofumi@mail.parknet.co.jp
 L:	linux-kernel@vger.kernel.org
-W:	http://bmrc.berkeley.edu/people/chaffee
 S:	Maintained
 
 VIA 82Cxxx AUDIO DRIVER

_
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: FAT statfs loop abort on read-error

[Christoph Hellwig]

On Tue, Jul 08, 2003 at 09:18:43PM +0900, OGAWA Hirofumi wrote:
> Thank you very much. I try it.
> 
> Please apply the following patch.

You've forgot to add yourself for the generic fat and msdosfs bits :)

Re: FAT statfs loop abort on read-error

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> writes:

> On Tue, Jul 08, 2003 at 09:18:43PM +0900, OGAWA Hirofumi wrote:
> > Thank you very much. I try it.
> > 
> > Please apply the following patch.
> 
> You've forgot to add yourself for the generic fat and msdosfs bits :)

Ok. Please apply the following patch.

 MAINTAINERS |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff -puN MAINTAINERS~maintainers-bk6 MAINTAINERS
--- linux-2.5.74/MAINTAINERS~maintainers-bk6	2003-07-08 23:47:05.000000000 +0900
+++ linux-2.5.74-hirofumi/MAINTAINERS	2003-07-08 23:47:36.000000000 +0900
@@ -2103,11 +2103,10 @@ L:	user-mode-linux-user@lists.sourceforg
 W:	http://user-mode-linux.sourceforge.net
 S:	Maintained
 	
-VFAT FILESYSTEM:
-P:	Gordon Chaffee
-M:	chaffee@cs.berkeley.edu
+FAT/VFAT/MSDOS FILESYSTEM:
+P:	OGAWA Hirofumi
+M:	hirofumi@mail.parknet.co.jp
 L:	linux-kernel@vger.kernel.org
-W:	http://bmrc.berkeley.edu/people/chaffee
 S:	Maintained
 
 VIA 82Cxxx AUDIO DRIVER

_
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: FAT statfs loop abort on read-error

[Jeff Garzik]

On Tue, Jul 08, 2003 at 12:54:48AM +0900, OGAWA Hirofumi wrote:
> > (I asked him to add a patch to MAINTAINTERS...)
> 
> Thank you. But honestly, I may not have skill enough.

The people who apply your patches think you do :)

	Jeff

Re: FAT statfs loop abort on read-error

[Sancho Dauskardt]

At 00:54 08.07.03 +0900, OGAWA Hirofumi wrote:
>"Randy.Dunlap" <rddunlap@osdl.org> writes:
>
> > On Fri, 04 Jul 2003 13:57:19 +0200 Sancho Dauskardt <sda@bdit.de> wrote:
> > |   when calling statfs on a volume that has been removed (without umount)
> > | fat_statfs() will attempt to read all sectors of the fat table quite 
> a few
> > | times (depending on the fat type, eg. FAT16 --> 256 times).
>
>Yes, fat driver of 2.4 ignore the many errors.
>
> > | Possible solution:
> > | 1. let default_fat_access return something like -2 on 'can't read' error.
> > | 2. Abort stafs loop on error.
> > | 3. return -EIO
> > |
> > | This would break mode fat_access calls. I could make a patch, but I 
> don't
> > | know what's going on with those cvf extensions (which seem to replace
> > | fat_access). Is dmsdos dead / can we ignore it ?
> > | Somewhere in the list archives, I found comments about the cvf stuff 
> being
> > | completely removed ?
>
>I don't know anybody ported dmsdos to 2.4. The cvf stuff was removed
>and many error handlings was fixed on 2.5.x. So, personally I think to
>remove the cvf stuff and backport the some parts of fat driver to 2.4
>is good.

OK, the 100k diff between 2.4.21/fs/fat and 2.5.74 didn't really help me 
understand what's really changed (other than the cvf removal).
Should I attempt to brute-force backport fs/fat/* in one large patch, or 
incrementally re-apply the 2.5 changes to 2.4 ?

Or, as you write 'some parts', which parts would that be ?

- sda

Re: FAT statfs loop abort on read-error

[OGAWA Hirofumi]

Sancho Dauskardt <sda@bdit.de> writes:

> >I don't know anybody ported dmsdos to 2.4. The cvf stuff was removed
> >and many error handlings was fixed on 2.5.x. So, personally I think to
> >remove the cvf stuff and backport the some parts of fat driver to 2.4
> >is good.
> 
> OK, the 100k diff between 2.4.21/fs/fat and 2.5.74 didn't really help
> me understand what's really changed (other than the cvf removal).
> Should I attempt to brute-force backport fs/fat/* in one large patch,
> or incrementally re-apply the 2.5 changes to 2.4 ?

I submited the some patch to marcelo several times about one year ago,
however, unfortunately those patches was ignored.

So, one large patch may not be applied. And incremental ways is more
safety, I think. (Probably, we need to address the difference of vfs
and umsdos)

> Or, as you write 'some parts', which parts would that be ?

I thought that the patches of only bug fix is probably easy to be
applied.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: FAT statfs loop abort on read-error

[Sancho Dauskardt]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

Hi,

  It's a while back, but now here's a patch that "works for me".
Once applied, we get a nice -EIO when doing a df / statfs() on a mounted 
FAT partition with removed media (without this would hang for minutes).

The change will affect anybody calling fat_access() (the cvf stuff, other 
fat-dependent modules).

Applies on 2.4.19 .. 2.4.22 for me.

Thanks,
- sda

At 23:46 08.07.03 +0900, OGAWA Hirofumi wrote:
>Sancho Dauskardt <sda@bdit.de> writes:
>
> > >I don't know anybody ported dmsdos to 2.4. The cvf stuff was removed
> > >and many error handlings was fixed on 2.5.x. So, personally I think to
> > >remove the cvf stuff and backport the some parts of fat driver to 2.4
> > >is good.
> >
> > OK, the 100k diff between 2.4.21/fs/fat and 2.5.74 didn't really help
> > me understand what's really changed (other than the cvf removal).
> > Should I attempt to brute-force backport fs/fat/* in one large patch,
> > or incrementally re-apply the 2.5 changes to 2.4 ?
>
>I submited the some patch to marcelo several times about one year ago,
>however, unfortunately those patches was ignored.
>
>So, one large patch may not be applied. And incremental ways is more
>safety, I think. (Probably, we need to address the difference of vfs
>and umsdos)
>
> > Or, as you write 'some parts', which parts would that be ?
>
>I thought that the patches of only bug fix is probably easy to be
>applied.


[-- Attachment #2: patch_2.4.21-fat-statfs-loop-abort.diff --]
[-- Type: application/octet-stream, Size: 3136 bytes --]

diff -urN -X dontdiff.txt linux-2.4.21-640l-net/fs/fat/cache.c linux-2.4.21/fs/fat/cache.c
--- linux-2.4.21-640l-net/fs/fat/cache.c	Fri Oct 12 22:48:42 2001
+++ linux-2.4.21/fs/fat/cache.c	Tue Sep  9 13:10:30 2003
@@ -56,7 +56,7 @@
 	b = MSDOS_SB(sb)->fat_start + (first >> sb->s_blocksize_bits);
 	if (!(bh = fat_bread(sb, b))) {
 		printk("bread in fat_access failed\n");
-		return 0;
+		return -2;		/* !!!!!!!!!!!!! */
 	}
 	if ((first >> sb->s_blocksize_bits) == (last >> sb->s_blocksize_bits)) {
 		bh2 = bh;
@@ -64,7 +68,7 @@
 		if (!(bh2 = fat_bread(sb, b+1))) {
 			fat_brelse(sb, bh);
 			printk("2nd bread in fat_access failed\n");
-			return 0;
+			return -2;	/* !!!!!!!! */
 		}
 	}
 	if (MSDOS_SB(sb)->fat_bits == 32) {
@@ -283,8 +287,8 @@
 	count = 0;
 	for (fat_cache_lookup(inode,cluster,&count,&nr); count < cluster;
 	    count++) {
-		if ((nr = fat_access(inode->i_sb,nr,-1)) == -1) return 0;
-		if (!nr) return 0;
+		nr = fat_access(inode->i_sb,nr,-1);
+		if (nr <= 0) return 0;		/* 0=last, -1=end -2=error */
 	}
 	fat_cache_add(inode,cluster,nr);
 	return nr;
@@ -328,7 +332,13 @@
 	last = 0;
 	while (skip--) {
 		last = nr;
-		if ((nr = fat_access(inode->i_sb,nr,-1)) == -1) return 0;
+		nr = fat_access(inode->i_sb,nr,-1);
+
+		if (nr == -1) return 0;
+		if (nr == -2) {
+			printk("fat_free: read error\n");
+			return -EIO;
+		}
 		if (!nr) {
 			printk("fat_free: skipped EOF\n");
 			return -EIO;
@@ -344,7 +354,7 @@
 		mark_inode_dirty(inode);
 	}
 	lock_fat(inode->i_sb);
-	while (nr != -1) {
+	while (nr >= 0) {		/* != -1 */
 		if (!(nr = fat_access(inode->i_sb,nr,0))) {
 			fat_fs_panic(inode->i_sb,"fat_free: deleting beyond EOF");
 			break;
diff -urN -X dontdiff.txt linux-2.4.21-640l-net/fs/fat/inode.c linux-2.4.21/fs/fat/inode.c
--- linux-2.4.21-640l-net/fs/fat/inode.c	Sat Aug  3 02:39:45 2002
+++ linux-2.4.21/fs/fat/inode.c	Tue Sep  9 13:10:28 2003
@@ -820,7 +820,7 @@
 
 int fat_statfs(struct super_block *sb,struct statfs *buf)
 {
-	int free,nr;
+	int free,nr,e;
        
 	if (MSDOS_SB(sb)->cvf_format &&
 	    MSDOS_SB(sb)->cvf_format->cvf_statfs)
@@ -832,8 +832,18 @@
 		free = MSDOS_SB(sb)->free_clusters;
 	else {
 		free = 0;
-		for (nr = 2; nr < MSDOS_SB(sb)->clusters+2; nr++)
-			if (!fat_access(sb,nr,-1)) free++;
+		for (nr = 2; nr < MSDOS_SB(sb)->clusters+2; nr++){
+			e = fat_access(sb,nr,-1);
+			if(!e)
+				free++;
+
+			if( e == -2 ){
+				printk("Can't fat_access sector %d, %d.\n",nr,e);
+				unlock_fat(sb);
+				return -EIO;
+			}
+		}
+		
 		MSDOS_SB(sb)->free_clusters = free;
 	}
 	unlock_fat(sb);
diff -urN -X dontdiff.txt linux-2.4.21-640l-net/fs/fat/misc.c linux-2.4.21/fs/fat/misc.c
--- linux-2.4.21-640l-net/fs/fat/misc.c	Fri Oct 12 22:48:42 2001
+++ linux-2.4.21/fs/fat/misc.c	Tue Sep  9 13:19:04 2003
@@ -165,7 +165,7 @@
 	if ((curr = MSDOS_I(inode)->i_start) != 0) {
 		fat_cache_lookup(inode, INT_MAX, &last, &curr);
 		file_cluster = last;
-		while (curr && curr != -1){
+		while ( curr > 0){	/* was (curr && curr != -1), now -2 == error */
 			file_cluster++;
 			if (!(curr = fat_access(sb, last = curr,-1))) {
 				fat_fs_panic(sb, "File without EOF");

Re: FAT statfs loop abort on read-error

[OGAWA Hirofumi]

Sancho Dauskardt <sda@bdit.de> writes:

> Hi,
> 
>   It's a while back, but now here's a patch that "works for me".
> Once applied, we get a nice -EIO when doing a df / statfs() on a
> mounted FAT partition with removed media (without this would hang for
> minutes).
> 
> The change will affect anybody calling fat_access() (the cvf stuff,
> other fat-dependent modules).

Yes, it looks like broken the fat_cvf API. So we should remove the
fat_cvf stuff, and it should return the real error code.

Even fat was broken. (fat_read_root etc. was not addressed)

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>