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
Damien Wyart <[email protected]> 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 <[email protected]>
OGAWA Hirofumi <[email protected]> writes:
> Damien Wyart <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> [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
Damien Wyart <[email protected]> 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 <[email protected]> [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 <[email protected]>
On Sat, 21 Oct 2006 19:38:49 +0200
Damien Wyart <[email protected]> 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;
On Sat, 21 Oct 2006 13:19:32 -0700
Andrew Morton <[email protected]> wrote:
> On Sat, 21 Oct 2006 19:38:49 +0200
> Damien Wyart <[email protected]> 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.
Andrew Morton <[email protected]> writes:
> On Sat, 21 Oct 2006 19:38:49 +0200
> Damien Wyart <[email protected]> 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 <[email protected]>
On Sat, Oct 21, 2006 at 01:24:31PM -0700, Andrew Morton wrote:
> On Sat, 21 Oct 2006 13:19:32 -0700
> Andrew Morton <[email protected]> wrote:
>
> > On Sat, 21 Oct 2006 19:38:49 +0200
> > Damien Wyart <[email protected]> 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?
On Sun, Oct 22, 2006 at 05:38:38AM +0900, OGAWA Hirofumi wrote:
> Andrew Morton <[email protected]> writes:
>
> > On Sat, 21 Oct 2006 19:38:49 +0200
> > Damien Wyart <[email protected]> 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?
Nick Piggin <[email protected]> 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 <[email protected]>
On Sun, Oct 22, 2006 at 06:11:36PM +0900, OGAWA Hirofumi wrote:
> Nick Piggin <[email protected]> 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
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 <[email protected]> [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