Re: [PATCH v2 1/5] add metadata_incore ioctl in vfs

[Shaohua Li <shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Wed, 2011-01-05 at 17:42 +0800, Arnd Bergmann wrote:
> On Wednesday 05 January 2011 03:17:16 Shaohua Li wrote:
> > On Tue, 2011-01-04 at 17:40 +0800, Arnd Bergmann wrote:
> > 
> > > Have you tried passing just a single metadata_incore_ent
> > > at the ioctl and looping in user space? I would guess the
> > > extra overhead of that would be small enough, but that might
> > > need to be measured.
> > metadata usually isn't continuous, so this means we have a lot of
> > metadata_incore_ent entries. And this is called at boot time and I want
> > to make the overhead as low as possible to not impact boot. Unless there
> > are certain reasons we can't use indirect pointers, I'd like to make
> > kernel return a vector of entries.
> 
> It's not a strict rule, but the indirect data passing is rather
> ugly and I'd only do that if the difference can be /measured/.
> 
> If the purpose is to speed up boot time by preloading metadata,
> the FIMETADATA_INCORE operations should of course not take a
> significant amount of time compared to the actual preloading,
> but as long as it's less than one percent of the time you need
> for the preload, I would just use the simpler interface.
ok, just have a measurement, the overhead is acceptable. I'll change the
code to just accept one entry.

> > @@ -882,6 +882,7 @@ COMPATIBLE_IOCTL(FIGETBSZ)
> >  /* 'X' - originally XFS but some now in the VFS */
> >  COMPATIBLE_IOCTL(FIFREEZE)
> >  COMPATIBLE_IOCTL(FITHAW)
> > +COMPATIBLE_IOCTL(FIMETADATA_INCORE)
> >  COMPATIBLE_IOCTL(KDGETKEYCODE)
> >  COMPATIBLE_IOCTL(KDSETKEYCODE)
> >  COMPATIBLE_IOCTL(KDGKBTYPE)
> 
> This change can go away as well.
I don't understand. adding a case statement in compat_sys_ioctl, so we will do
compat_ioctl_check_table(). If I add COMPATIBLE_IOCTL(), then the check
will success, we will go to the found_handler code path and execute
do_vfs_ioctl, which is what we want. if not adding COMPATIBLE_IOCTL(),
the check will fail, and in any case, we will go to the out_fput code
path, so our ioctl does nothing.

> Two more general comments:
> 
> - You probably want to add the ioctls to file_ioctl instead of do_vfs_ioctl,
>   so you don't add another case statement to the common path.
> 
> - I don't know if there are any rules for what should be an ioctl or an
>   fcntl, we're rather inconsistent about this. If you have found a good
>   reason for making it an ioctl, just put that into the changelog so we
>   can refer to it next time.
it can be applied to a directory too. I thought file_ioctl or fcntl is
for file.

Thanks,
Shaohua