lkml.org 
[lkml]   [2007]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] fat: Editions to support fat_fallocate()
Date
"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>


\
 
 \ /
  Last update: 2007-12-23 13:19    [W:0.053 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site