2.6.19-rc2-mm2 : empty files on vfat file system

2.6.19-rc2-mm2 : empty files on vfat file system

[Damien Wyart]

Hello,

I have noticed something strange (and bad :) since using 2.6.19-rc2-mm2
(the problem is NOT present on 2.6.19-rc2-mm1 ; do not know for
mainline, I have not been able to test yet, but I think there have not
been recent changes in this area) : writing a file to a vfat
fs (fat 32) writes it, but with size 0 and no content. All this is
silent : no error message, nothing in the logs. After several attempts,
I checked the fs with fsck.vfat and it reported errors about some of the
files and told it was truncating them to size 0 (but their displayed
size was already 0, btw).


I hope this will ring a bell for some of you ? Maybe related to the
patch about strange bug messages about inodes in mm1 ?

-- 
Damien Wyart

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[OGAWA Hirofumi]

Damien Wyart <damien.wyart@free.fr> writes:

> I have noticed something strange (and bad :) since using 2.6.19-rc2-mm2
> (the problem is NOT present on 2.6.19-rc2-mm1 ; do not know for
> mainline, I have not been able to test yet, but I think there have not
> been recent changes in this area) : writing a file to a vfat
> fs (fat 32) writes it, but with size 0 and no content. All this is
> silent : no error message, nothing in the logs. After several attempts,
> I checked the fs with fsck.vfat and it reported errors about some of the
> files and told it was truncating them to size 0 (but their displayed
> size was already 0, btw).

diff -puN fs/fat/inode.c~fs-prepare_write-fixes fs/fat/inode.c
--- a/fs/fat/inode.c~fs-prepare_write-fixes
+++ a/fs/fat/inode.c
@@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
 			    unsigned from, unsigned to)
 {
 	struct inode *inode = page->mapping->host;
-	int err = generic_commit_write(file, page, from, to);
+	int err;
+	if (to - from > 0)
+		return 0;
+
+	err = generic_commit_write(file, page, from, to);
 	if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
 		inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
 		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;

This change does't update ->i_size. Could you just delete, and test it?
Anyway, this seems wrong even if it's "if ((to - from) == 0)".  The zero
range is valid for cont_prepare_write()...
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Damien Wyart <damien.wyart@free.fr> writes:
>
>> I have noticed something strange (and bad :) since using 2.6.19-rc2-mm2
>> (the problem is NOT present on 2.6.19-rc2-mm1 ; do not know for
>> mainline, I have not been able to test yet, but I think there have not
>> been recent changes in this area) : writing a file to a vfat
>> fs (fat 32) writes it, but with size 0 and no content. All this is
>> silent : no error message, nothing in the logs. After several attempts,
>> I checked the fs with fsck.vfat and it reported errors about some of the
>> files and told it was truncating them to size 0 (but their displayed
>> size was already 0, btw).
>
> diff -puN fs/fat/inode.c~fs-prepare_write-fixes fs/fat/inode.c
> --- a/fs/fat/inode.c~fs-prepare_write-fixes
> +++ a/fs/fat/inode.c
> @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
>  			    unsigned from, unsigned to)
>  {
>  	struct inode *inode = page->mapping->host;
> -	int err = generic_commit_write(file, page, from, to);
> +	int err;
> +	if (to - from > 0)
> +		return 0;
> +
> +	err = generic_commit_write(file, page, from, to);
>  	if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
>  		inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
>  		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
>
> This change does't update ->i_size. Could you just delete, and test it?
> Anyway, this seems wrong even if it's "if ((to - from) == 0)".  The zero
> range is valid for cont_prepare_write()...

s/cont_prepare_write/generic_cont_expand/.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Damien Wyart]

> > I have noticed something strange (and bad :) since using
> > 2.6.19-rc2-mm2 (the problem is NOT present on 2.6.19-rc2-mm1 ; do
> > not know for mainline, I have not been able to test yet, but I think
> > there have not been recent changes in this area) : writing a file to
> > a vfat fs (fat 32) writes it, but with size 0 and no content.

* OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> [2006-10-21 22:24]:
> diff -puN fs/fat/inode.c~fs-prepare_write-fixes fs/fat/inode.c
> --- a/fs/fat/inode.c~fs-prepare_write-fixes
> +++ a/fs/fat/inode.c
> @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
>  			    unsigned from, unsigned to)
>  {
>  	struct inode *inode = page->mapping->host;
> -	int err = generic_commit_write(file, page, from, to);
> +	int err;
> +	if (to - from > 0)
> +		return 0;
> +
> +	err = generic_commit_write(file, page, from, to);
>  	if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
>  		inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
>  		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;

> This change does't update ->i_size. Could you just delete, and test
> it? Anyway, this seems wrong even if it's "if ((to - from) == 0)".

Reverting the change makes the problem go away. But I do not know if
this is safe wrt the fs-prepare_write-fixes patch.

-- 
Damien Wyart

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[OGAWA Hirofumi]

Damien Wyart <damien.wyart@free.fr> writes:

>> > I have noticed something strange (and bad :) since using
>> > 2.6.19-rc2-mm2 (the problem is NOT present on 2.6.19-rc2-mm1 ; do
>> > not know for mainline, I have not been able to test yet, but I think
>> > there have not been recent changes in this area) : writing a file to
>> > a vfat fs (fat 32) writes it, but with size 0 and no content.
>
> * OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> [2006-10-21 22:24]:
>> diff -puN fs/fat/inode.c~fs-prepare_write-fixes fs/fat/inode.c
>> --- a/fs/fat/inode.c~fs-prepare_write-fixes
>> +++ a/fs/fat/inode.c
>> @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
>>  			    unsigned from, unsigned to)
>>  {
>>  	struct inode *inode = page->mapping->host;
>> -	int err = generic_commit_write(file, page, from, to);
>> +	int err;
>> +	if (to - from > 0)
>> +		return 0;
>> +
>> +	err = generic_commit_write(file, page, from, to);
>>  	if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
>>  		inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
>>  		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
>
>> This change does't update ->i_size. Could you just delete, and test
>> it? Anyway, this seems wrong even if it's "if ((to - from) == 0)".
>
> Reverting the change makes the problem go away. But I do not know if
> this is safe wrt the fs-prepare_write-fixes patch.

Yes, maybe another patch will be needed. However, I don't know the
background of this change. I'll make time and see what happened.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Andrew Morton]

On Sat, 21 Oct 2006 19:38:49 +0200
Damien Wyart <damien.wyart@free.fr> wrote:

> > --- a/fs/fat/inode.c~fs-prepare_write-fixes
> > +++ a/fs/fat/inode.c
> > @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
> >  			    unsigned from, unsigned to)
> >  {
> >  	struct inode *inode = page->mapping->host;
> > -	int err = generic_commit_write(file, page, from, to);
> > +	int err;
> > +	if (to - from > 0)
> > +		return 0;
> > +

That should have been

	if (to - from == 0)
		return 0;

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Andrew Morton]

On Sat, 21 Oct 2006 13:19:32 -0700
Andrew Morton <akpm@osdl.org> wrote:

> On Sat, 21 Oct 2006 19:38:49 +0200
> Damien Wyart <damien.wyart@free.fr> wrote:
> 
> > > --- a/fs/fat/inode.c~fs-prepare_write-fixes
> > > +++ a/fs/fat/inode.c
> > > @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
> > >  			    unsigned from, unsigned to)
> > >  {
> > >  	struct inode *inode = page->mapping->host;
> > > -	int err = generic_commit_write(file, page, from, to);
> > > +	int err;
> > > +	if (to - from > 0)
> > > +		return 0;
> > > +
> 
> That should have been
> 
> 	if (to - from == 0)
> 		return 0;

otoh, it's still wrong that we're not updating i_size.  We happen to know
that the caller will retry without dropping i_mutex, but it's a bit
incestuous.

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[OGAWA Hirofumi]

Andrew Morton <akpm@osdl.org> writes:

> On Sat, 21 Oct 2006 19:38:49 +0200
> Damien Wyart <damien.wyart@free.fr> wrote:
>
>> > --- a/fs/fat/inode.c~fs-prepare_write-fixes
>> > +++ a/fs/fat/inode.c
>> > @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
>> >  			    unsigned from, unsigned to)
>> >  {
>> >  	struct inode *inode = page->mapping->host;
>> > -	int err = generic_commit_write(file, page, from, to);
>> > +	int err;
>> > +	if (to - from > 0)
>> > +		return 0;
>> > +
>
> That should have been
>
> 	if (to - from == 0)
> 		return 0;

As I said in this thread, generic_cont_expand() uses "to == from".
Should we fix generic_cont_expand() instead? I don't know the
background of this patch.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Nick Piggin]

On Sat, Oct 21, 2006 at 01:24:31PM -0700, Andrew Morton wrote:
> On Sat, 21 Oct 2006 13:19:32 -0700
> Andrew Morton <akpm@osdl.org> wrote:
> 
> > On Sat, 21 Oct 2006 19:38:49 +0200
> > Damien Wyart <damien.wyart@free.fr> wrote:
> > 
> > > > --- a/fs/fat/inode.c~fs-prepare_write-fixes
> > > > +++ a/fs/fat/inode.c
> > > > @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
> > > >  			    unsigned from, unsigned to)
> > > >  {
> > > >  	struct inode *inode = page->mapping->host;
> > > > -	int err = generic_commit_write(file, page, from, to);
> > > > +	int err;
> > > > +	if (to - from > 0)
> > > > +		return 0;
> > > > +
> > 
> > That should have been
> > 
> > 	if (to - from == 0)
> > 		return 0;
> 
> otoh, it's still wrong that we're not updating i_size.  We happen to know
> that the caller will retry without dropping i_mutex, but it's a bit
> incestuous.

It can possibly fail for example if the source buffer gets unmapped.
However in the length == 0 case, that signals a failure to write anything
so we needn't update i_size, I think?

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Nick Piggin]

On Sun, Oct 22, 2006 at 05:38:38AM +0900, OGAWA Hirofumi wrote:
> Andrew Morton <akpm@osdl.org> writes:
> 
> > On Sat, 21 Oct 2006 19:38:49 +0200
> > Damien Wyart <damien.wyart@free.fr> wrote:
> >
> >> > --- a/fs/fat/inode.c~fs-prepare_write-fixes
> >> > +++ a/fs/fat/inode.c
> >> > @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
> >> >  			    unsigned from, unsigned to)
> >> >  {
> >> >  	struct inode *inode = page->mapping->host;
> >> > -	int err = generic_commit_write(file, page, from, to);
> >> > +	int err;
> >> > +	if (to - from > 0)
> >> > +		return 0;
> >> > +
> >
> > That should have been
> >
> > 	if (to - from == 0)
> > 		return 0;
> 
> As I said in this thread, generic_cont_expand() uses "to == from".
> Should we fix generic_cont_expand() instead? I don't know the
> background of this patch.

OK I have to write an RFC for various fs developers, so I'll be sure
to include you.

We want to be able to pass in a short (possibly zero) commit_write
length if the page data can not be fully copied.

generic_cont_expand seems to be using that as a shorthand for
"update the i_size but don't mark anything uptdoate"? If so, I think
it would be nice to fix it. Why does it even need to go through the
prepare/commit? Why not make __generic_cont_expand a standalone
function, and call that from cont_prepare_write? 

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[OGAWA Hirofumi]

Nick Piggin <npiggin@suse.de> writes:

> On Sun, Oct 22, 2006 at 05:38:38AM +0900, OGAWA Hirofumi wrote:
>> 
>> As I said in this thread, generic_cont_expand() uses "to == from".
>> Should we fix generic_cont_expand() instead? I don't know the
>> background of this patch.
>
> OK I have to write an RFC for various fs developers, so I'll be sure
> to include you.
>
> We want to be able to pass in a short (possibly zero) commit_write
> length if the page data can not be fully copied.

Thanks. So, if the range is zero, what should fs driver do?

> generic_cont_expand seems to be using that as a shorthand for
> "update the i_size but don't mark anything uptdoate"? If so, I think
> it would be nice to fix it. Why does it even need to go through the
> prepare/commit?

It's not only for updating ->i_size.

__generic_cont_expand() is used for expanding ->i_size by trucate(2).
In FAT case, it needs to fill the hole by zero and dirty it before
write new ->i_size. The ->prepare_write() does it, and ->commit_write()
writes ->i_size and dirty inode in __generic_cont_expand().

Anyway, if it's required, maybe we would be able to change
__generic_cont_expand().
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Nick Piggin]

On Sun, Oct 22, 2006 at 06:11:36PM +0900, OGAWA Hirofumi wrote:
> Nick Piggin <npiggin@suse.de> writes:
> 
> > On Sun, Oct 22, 2006 at 05:38:38AM +0900, OGAWA Hirofumi wrote:
> >> 
> >> As I said in this thread, generic_cont_expand() uses "to == from".
> >> Should we fix generic_cont_expand() instead? I don't know the
> >> background of this patch.
> >
> > OK I have to write an RFC for various fs developers, so I'll be sure
> > to include you.
> >
> > We want to be able to pass in a short (possibly zero) commit_write
> > length if the page data can not be fully copied.
> 
> Thanks. So, if the range is zero, what should fs driver do?

The fs driver should ensure that any userspace access to the
filesystem/pagecache should behave as if no prepare_write were
called at all.

That includes deallocating or zeroing any potentially allocated
but uninitialized blocks so they won't get read in from disk in future.
But that isn't handled properly yet though, so we can probably solve
that in the core kernel rather than your filesystem. Discussion about
this should go to the new mail I've just posted.


> > generic_cont_expand seems to be using that as a shorthand for
> > "update the i_size but don't mark anything uptdoate"? If so, I think
> > it would be nice to fix it. Why does it even need to go through the
> > prepare/commit?
> 
> It's not only for updating ->i_size.
> 
> __generic_cont_expand() is used for expanding ->i_size by trucate(2).
> In FAT case, it needs to fill the hole by zero and dirty it before
> write new ->i_size. The ->prepare_write() does it, and ->commit_write()
> writes ->i_size and dirty inode in __generic_cont_expand().
> 
> Anyway, if it's required, maybe we would be able to change
> __generic_cont_expand().

Yes I see, after reading it a bit. I'll take a look at it and try
to work out if anything needs fixing.

Thanks,
Nick

Re: 2.6.19-rc2-mm2 : empty files on vfat file system

[Mel Gorman]

On (21/10/06 19:38), Damien Wyart didst pronounce:
> > > I have noticed something strange (and bad :) since using
> > > 2.6.19-rc2-mm2 (the problem is NOT present on 2.6.19-rc2-mm1 ; do
> > > not know for mainline, I have not been able to test yet, but I think
> > > there have not been recent changes in this area) : writing a file to
> > > a vfat fs (fat 32) writes it, but with size 0 and no content.
> 
> * OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> [2006-10-21 22:24]:
> > diff -puN fs/fat/inode.c~fs-prepare_write-fixes fs/fat/inode.c
> > --- a/fs/fat/inode.c~fs-prepare_write-fixes
> > +++ a/fs/fat/inode.c
> > @@ -150,7 +150,11 @@ static int fat_commit_write(struct file 
> >  			    unsigned from, unsigned to)
> >  {
> >  	struct inode *inode = page->mapping->host;
> > -	int err = generic_commit_write(file, page, from, to);
> > +	int err;
> > +	if (to - from > 0)
> > +		return 0;
> > +
> > +	err = generic_commit_write(file, page, from, to);
> >  	if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
> >  		inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
> >  		MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
> 
> > This change does't update ->i_size. Could you just delete, and test
> > it? Anyway, this seems wrong even if it's "if ((to - from) == 0)".
> 
> Reverting the change makes the problem go away. But I do not know if
> this is safe wrt the fs-prepare_write-fixes patch.
> 

I don't know about the fix, but the issue is pretty serious for IA64 and the
EFI bootloader. On the IA64 I have access to, the bootloader and related
files are stored on a VFAT file system so when an automated system ran a
simple boot-test, the bootloader was blown away as a result.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab