vfat vs msdos and -o flush

vfat vs msdos and -o flush

[Christoph Hellwig]

Commit ae78bf9c4f5fde3c67e2829505f195d7347ce3e4 added a -o flush
option to the fat driver back in 2006, which causes asynchronous
writeout after I/O operations ASAP.  This already is quite hacky
and not something I like very much, but even worse the option
is accepted when using the more common vfat filesysten type, but
only actually implemented for the less common legacy msdos
filesystem type.

This tells us two lessons:

  - the -o flush option probably never got a whole lot of exposure
    and users didn't notice it doesn't work.  We might as well just
    remove it again
  - having the highlevel inode operations duplicated between msdos
    and vfat is probably a bad idea, and we should only branch out
    for the very low-level directory entry manipulations.

Re: vfat vs msdos and -o flush

[OGAWA Hirofumi]

Christoph Hellwig <hch@lst.de> writes:

> Commit ae78bf9c4f5fde3c67e2829505f195d7347ce3e4 added a -o flush
> option to the fat driver back in 2006, which causes asynchronous
> writeout after I/O operations ASAP.  This already is quite hacky
> and not something I like very much, but even worse the option
> is accepted when using the more common vfat filesysten type, but
> only actually implemented for the less common legacy msdos
> filesystem type.
>
> This tells us two lessons:
>
>   - the -o flush option probably never got a whole lot of exposure
>     and users didn't notice it doesn't work.  We might as well just
>     remove it again

Sigh. Chris, could you handle this?

>   - having the highlevel inode operations duplicated between msdos
>     and vfat is probably a bad idea, and we should only branch out
>     for the very low-level directory entry manipulations.

I know you are finding the reason to merge those. ;)

Well, what code are you suggesting? (BTW, this is not meaning "Please
send a patch.").  Can we do it without the user visible change? And if
it has the user visible change, what is benefit to users?

I know, if we kill original design decision with the user visible
change, we can write much more faster/cleaner code. (e.g. some I/O can
be reduced for directory. etc.) But, for fatfs, I'm thinking there is no
benefit to take a pain by the user visible change, at least for now.

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

Re: vfat vs msdos and -o flush

[OGAWA Hirofumi]

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

> Christoph Hellwig <hch@lst.de> writes:
>
>> Commit ae78bf9c4f5fde3c67e2829505f195d7347ce3e4 added a -o flush
>> option to the fat driver back in 2006, which causes asynchronous
>> writeout after I/O operations ASAP.  This already is quite hacky
>> and not something I like very much, but even worse the option
>> is accepted when using the more common vfat filesysten type, but
>> only actually implemented for the less common legacy msdos
>> filesystem type.
>>
>> This tells us two lessons:
>>
>>   - the -o flush option probably never got a whole lot of exposure
>>     and users didn't notice it doesn't work.  We might as well just
>>     remove it again
>
> Sigh. Chris, could you handle this?

It seems there is no answer, so I have no objection to drop it...

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

Re: vfat vs msdos and -o flush

[Christoph Hellwig]

On Wed, Mar 03, 2010 at 10:36:21PM +0900, OGAWA Hirofumi wrote:
> >   - having the highlevel inode operations duplicated between msdos
> >     and vfat is probably a bad idea, and we should only branch out
> >     for the very low-level directory entry manipulations.
> 
> I know you are finding the reason to merge those. ;)
> 
> Well, what code are you suggesting? (BTW, this is not meaning "Please
> send a patch.").  Can we do it without the user visible change? And if
> it has the user visible change, what is benefit to users?
> 
> I know, if we kill original design decision with the user visible
> change, we can write much more faster/cleaner code. (e.g. some I/O can
> be reduced for directory. etc.) But, for fatfs, I'm thinking there is no
> benefit to take a pain by the user visible change, at least for now.

There's a couple of things I have in mind:

 - first move fat and msdos into the same module to make integration
   easier.  Keep MODULE_ALIASes for vfat and msdos to allow old module
   (auto-)load setups working.  Move the setup for the two
   file_system_type structures to super.c
 - look at the spurious differences between namei_msdos.c and
   namei_vfat.c.  Here's a few I've noticed and I'll have patches for
   soon hopefully:

     - msdos only updates c and mtime in msdos_add_entry, but vfat
       also updates atime
     - msdos only updates ctime in rmdir and unlink , vfat otoh only
       updates mtime and atime
     - vfat updates the ctime on the new inode in rename, msdos
       doesn't.

  - udate i_version consistently in msdos, too (probably not needed
    without nfs exporting, but just to make the code similar)
  - unify the vfat_add_entry/msdos_add_entry prototypes and make them
    function pointers hanging off the superblock information
  - move the hidden file handling and msdos_format_name into a helper
    and make that a function pointer hanging off the superblock,
    with a no-op implementation for vfat
  - unify the prototypes of msdos_find and vfat_find and make it
    a function pointer hanging off the superblock

At this point basically all the inode operations are the same for
both and can be merged, spreading out via the three operations
mentioned above.  If we don't want the indirection it could also
be done using if/else statements in a small helper.  The only one
I haven't analyzed in detail yet is rename, because it looks too
different and will need some refactoring first.

Re: vfat vs msdos and -o flush

[OGAWA Hirofumi]

Christoph Hellwig <hch@lst.de> writes:

> There's a couple of things I have in mind:
>
>  - first move fat and msdos into the same module to make integration
>    easier.  Keep MODULE_ALIASes for vfat and msdos to allow old module
>    (auto-)load setups working.  Move the setup for the two
>    file_system_type structures to super.c
>  - look at the spurious differences between namei_msdos.c and
>    namei_vfat.c.  Here's a few I've noticed and I'll have patches for
>    soon hopefully:
>
>      - msdos only updates c and mtime in msdos_add_entry, but vfat
>        also updates atime
>      - msdos only updates ctime in rmdir and unlink , vfat otoh only
>        updates mtime and atime
>      - vfat updates the ctime on the new inode in rename, msdos
>        doesn't.

Originally msdos didn't have atime field, so atime is intended more or
less. Others are I'm not pretty sure, so, didn't touch original
one. However, I have no objection to others.

>   - udate i_version consistently in msdos, too (probably not needed
>     without nfs exporting, but just to make the code similar)

Yes, nfs export of msdos would be totally broken (although vfat is also
broken more or less due to stable ino issue). However, it sounds good.

>   - unify the vfat_add_entry/msdos_add_entry prototypes and make them
>     function pointers hanging off the superblock information
>   - move the hidden file handling and msdos_format_name into a helper
>     and make that a function pointer hanging off the superblock,
>     with a no-op implementation for vfat
>   - unify the prototypes of msdos_find and vfat_find and make it
>     a function pointer hanging off the superblock
>
> At this point basically all the inode operations are the same for
> both and can be merged, spreading out via the three operations
> mentioned above.  If we don't want the indirection it could also
> be done using if/else statements in a small helper.  The only one
> I haven't analyzed in detail yet is rename, because it looks too
> different and will need some refactoring first.

I think this is meaning to add top level layer using existent dentry
handling basically. After all, we would have to care those original
strange difference (sigh) in internal though, however this sounds good
to me.

[BTW, I was thinking in past, just kill msdos and adds new shortname
only option to vfat. ;)]

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

Re: vfat vs msdos and -o flush

[Christoph Hellwig]

On Sun, Mar 07, 2010 at 11:15:42PM +0900, OGAWA Hirofumi wrote:
> [BTW, I was thinking in past, just kill msdos and adds new shortname
> only option to vfat. ;)]

Sounds even better to me.  But I'm not uptodate on vfat internals enough
to decide whether that's easily doable.  We still can do that after
the consolidation I suggested above.

Re: vfat vs msdos and -o flush

[Chris Mason]

On Wed, Mar 03, 2010 at 10:36:21PM +0900, OGAWA Hirofumi wrote:
> Christoph Hellwig <hch@lst.de> writes:
> 
> > Commit ae78bf9c4f5fde3c67e2829505f195d7347ce3e4 added a -o flush
> > option to the fat driver back in 2006, which causes asynchronous
> > writeout after I/O operations ASAP.  This already is quite hacky
> > and not something I like very much, but even worse the option
> > is accepted when using the more common vfat filesysten type, but
> > only actually implemented for the less common legacy msdos
> > filesystem type.
> >
> > This tells us two lessons:
> >
> >   - the -o flush option probably never got a whole lot of exposure
> >     and users didn't notice it doesn't work.  We might as well just
> >     remove it again
> 
> Sigh. Chris, could you handle this?

I'm not against removing it.  The option was added to handle desktop
workloads where mount -o sync was too slow but people still wanted to be
able to rip out the usb stick..  Added Jeff to the cc list, is this still
in use?

Now that we have per-bdi dirty accounting, I think we're much much
better off to set the dirty limit on the bdi to 1 byte or something and
just let the page cache code do it all for us.

-chris

Re: vfat vs msdos and -o flush

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/09/2010 08:42 AM, Chris Mason wrote:
> On Wed, Mar 03, 2010 at 10:36:21PM +0900, OGAWA Hirofumi wrote:
>> Christoph Hellwig <hch@lst.de> writes:
>>
>>> Commit ae78bf9c4f5fde3c67e2829505f195d7347ce3e4 added a -o flush
>>> option to the fat driver back in 2006, which causes asynchronous
>>> writeout after I/O operations ASAP.  This already is quite hacky
>>> and not something I like very much, but even worse the option
>>> is accepted when using the more common vfat filesysten type, but
>>> only actually implemented for the less common legacy msdos
>>> filesystem type.
>>>
>>> This tells us two lessons:
>>>
>>>   - the -o flush option probably never got a whole lot of exposure
>>>     and users didn't notice it doesn't work.  We might as well just
>>>     remove it again
>>
>> Sigh. Chris, could you handle this?
> 
> I'm not against removing it.  The option was added to handle desktop
> workloads where mount -o sync was too slow but people still wanted to be
> able to rip out the usb stick..  Added Jeff to the cc list, is this still
> in use?
> 
> Now that we have per-bdi dirty accounting, I think we're much much
> better off to set the dirty limit on the bdi to 1 byte or something and
> just let the page cache code do it all for us.

Yes, we still use it on openSUSE, but we haven't gotten any reports
about it being broken. I expect it's because it's only partly broken and
users haven't run into breakage with pure metadata operations. The inode
still gets written back when the file is closed.

If this can be solved differently, perfect.

- -Jeff

- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/

iEYEARECAAYFAkuX8NYACgkQLPWxlyuTD7KERQCfQJKKxZ98XfLIe7T9qcyhtkpY
j54An2PP5zz9UU+TXaIhNC0dQdY81w2g
=2jPD
-----END PGP SIGNATURE-----