* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 17:38 ` Damien Wyart
@ 2006-10-21 20:17 ` OGAWA Hirofumi
2006-10-21 20:19 ` Andrew Morton
2006-10-23 20:39 ` Mel Gorman
2 siblings, 0 replies; 13+ messages in thread
From: OGAWA Hirofumi @ 2006-10-21 20:17 UTC (permalink / raw)
To: Damien Wyart; +Cc: linux-kernel, Nick Piggin, Andrew Morton
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 17:38 ` Damien Wyart
2006-10-21 20:17 ` OGAWA Hirofumi
@ 2006-10-21 20:19 ` Andrew Morton
2006-10-21 20:24 ` Andrew Morton
2006-10-21 20:38 ` OGAWA Hirofumi
2006-10-23 20:39 ` Mel Gorman
2 siblings, 2 replies; 13+ messages in thread
From: Andrew Morton @ 2006-10-21 20:19 UTC (permalink / raw)
To: Damien Wyart; +Cc: OGAWA Hirofumi, linux-kernel, Nick Piggin
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;
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 20:19 ` Andrew Morton
@ 2006-10-21 20:24 ` Andrew Morton
2006-10-22 1:34 ` Nick Piggin
2006-10-21 20:38 ` OGAWA Hirofumi
1 sibling, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2006-10-21 20:24 UTC (permalink / raw)
To: Damien Wyart, OGAWA Hirofumi, linux-kernel, Nick Piggin
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 20:24 ` Andrew Morton
@ 2006-10-22 1:34 ` Nick Piggin
0 siblings, 0 replies; 13+ messages in thread
From: Nick Piggin @ 2006-10-22 1:34 UTC (permalink / raw)
To: Andrew Morton; +Cc: Damien Wyart, OGAWA Hirofumi, linux-kernel
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 20:19 ` Andrew Morton
2006-10-21 20:24 ` Andrew Morton
@ 2006-10-21 20:38 ` OGAWA Hirofumi
2006-10-22 1:50 ` Nick Piggin
1 sibling, 1 reply; 13+ messages in thread
From: OGAWA Hirofumi @ 2006-10-21 20:38 UTC (permalink / raw)
To: Andrew Morton; +Cc: Damien Wyart, linux-kernel, Nick Piggin
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>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 20:38 ` OGAWA Hirofumi
@ 2006-10-22 1:50 ` Nick Piggin
2006-10-22 9:11 ` OGAWA Hirofumi
0 siblings, 1 reply; 13+ messages in thread
From: Nick Piggin @ 2006-10-22 1:50 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: Andrew Morton, Damien Wyart, linux-kernel
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?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-22 1:50 ` Nick Piggin
@ 2006-10-22 9:11 ` OGAWA Hirofumi
2006-10-22 9:25 ` Nick Piggin
0 siblings, 1 reply; 13+ messages in thread
From: OGAWA Hirofumi @ 2006-10-22 9:11 UTC (permalink / raw)
To: Nick Piggin; +Cc: Andrew Morton, Damien Wyart, linux-kernel
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-22 9:11 ` OGAWA Hirofumi
@ 2006-10-22 9:25 ` Nick Piggin
0 siblings, 0 replies; 13+ messages in thread
From: Nick Piggin @ 2006-10-22 9:25 UTC (permalink / raw)
To: OGAWA Hirofumi; +Cc: Andrew Morton, Damien Wyart, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: 2.6.19-rc2-mm2 : empty files on vfat file system
2006-10-21 17:38 ` Damien Wyart
2006-10-21 20:17 ` OGAWA Hirofumi
2006-10-21 20:19 ` Andrew Morton
@ 2006-10-23 20:39 ` Mel Gorman
2 siblings, 0 replies; 13+ messages in thread
From: Mel Gorman @ 2006-10-23 20:39 UTC (permalink / raw)
To: Damien Wyart; +Cc: OGAWA Hirofumi, linux-kernel, Nick Piggin, Andrew Morton
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
^ permalink raw reply [flat|nested] 13+ messages in thread