"Grant Likely" <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/