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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
