Re: [PATCH] fat: Editions to support fat_fallocate()

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

"Grant Likely" <grant.likely@secretlab.ca> writes:

>> +/*
>> + * preallocate space for a file. This implements fat fallocate inode
>> + * operation, which gets called from sys_fallocate system call. User
>> + * space requests len bytes at offset.
>> + */
>> +long fat_fallocate(struct inode *inode, int mode, loff_t offset, loff_t len)

This should be "static".

>> +{
>> +       int ret = 0;
>> +       loff_t filesize = inode->i_size;
>> +
>> +       /* preallocation to directories is currently not supported */
>> +       if (S_ISDIR(inode->i_mode)) {
>> +               printk(KERN_ERR
>> +               "fat_fallocate(): Directory prealloc not supported\n");
>
> This printk is probably not needed, or at least make it a pr_debug()

Yes. Please remove printk().

>> +               return -ENODEV;
>> +       }
>> +
>> +       if ((offset + len) <= MSDOS_I(inode)->mmu_private) {
>> +               printk(KERN_INFO
>> +                       "fat_fallocate():Blocks already allocated\n");
>
> Drop the printk() here.  It will cause a write to the system log
> everytime userspace does an unnecessary fat_fallocate().  That becomes
> a performance hit which I don't think we want.

Yes. And it should be ->i_size, not ->mmu_private.

>> +       if ((offset + len) > MSDOS_I(inode)->mmu_private) {

Ditto. This should also be ->i_size. Furthermore, this check should be
under ->i_mutex, otherwise others may expand ->i_size before this already.

>> +               mutex_lock(&inode->i_mutex);
>> +               ret = fat_cont_expand(inode, (offset + len));
>> +               if (ret) {
>> +                       printk(KERN_ERR
>> +                               "fat_fallocate():fat_cont_expand() error\n");
>> +                       mutex_unlock(&inode->i_mutex);
>> +                       return ret;
>> +               }
>> +               mutex_unlock(&inode->i_mutex);
>> +       }
>> +       if (mode & FALLOC_FL_KEEP_SIZE) {
>> +               mutex_lock(&inode->i_mutex);
>> +               i_size_write(inode, filesize);
>> +               mutex_unlock(&inode->i_mutex);
>> +       }
>
> Race condition.  The file is increased in size and then returned to
> the original size if FALLOC_FL_KEEP_SIZE is set, but the lock is
> dropped then reobtained between increasing the size and restoring to
> the original.  Another file operation can occur between the two
> operations.  Badness!
>
> However, digging further, when FALLOC_FL_KEEP_SIZE is set, I don't
> think fat_cont_expand() has the behaviour that we want to implement.
> When that flag is set, I think we simply want to add clusters
> associated with the file to the FAT.  We don't want to clear them or
> map them into the page cache yet (that should be done when the
> filesize is increased for real).
>
> I believe a call to fat_allocate_clusters() is all that is needed in
> this case.  Hirofumi, please correct me if I'm wrong.

Right. And we need to care the limitation on FAT specification (compatibility).

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