Re: [PATCH] fat: Editions to support fat_fallocate()
"Grant Likely" <[EMAIL PROTECTED]> writes: > On 12/23/07, OGAWA Hirofumi <[EMAIL PROTECTED]> wrote: >> "Grant Likely" <[EMAIL PROTECTED]> writes: >> > >> > 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). > > I not sure I fully understand what you mean. Can you please > elaborate? Are you referring to whether on not it will break other > FAT implementations if a file has more clusters allocated than it > needs? If so, how do we decide whether or not it is acceptable? [Sorry for long delay. I was on vacation.] Probably we need to check how Widnows behave in some situations. E.g. if we store the longer cluster-chain than i_size (in the case of FALLOC_FL_KEEP_SIZE), the driver will be seen like corrupted files. Because we doesn't know the file is whether file was "fallocate" or not after reboot. At least, I think current linux implementation will detect it as corrupted file (filesystem). So we have to handle somehow those situations. Also I think we'll need to more investigate problem like this. 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
Grant Likely [EMAIL PROTECTED] writes: On 12/23/07, OGAWA Hirofumi [EMAIL PROTECTED] wrote: Grant Likely [EMAIL PROTECTED] writes: 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). I not sure I fully understand what you mean. Can you please elaborate? Are you referring to whether on not it will break other FAT implementations if a file has more clusters allocated than it needs? If so, how do we decide whether or not it is acceptable? [Sorry for long delay. I was on vacation.] Probably we need to check how Widnows behave in some situations. E.g. if we store the longer cluster-chain than i_size (in the case of FALLOC_FL_KEEP_SIZE), the driver will be seen like corrupted files. Because we doesn't know the file is whether file was fallocate or not after reboot. At least, I think current linux implementation will detect it as corrupted file (filesystem). So we have to handle somehow those situations. Also I think we'll need to more investigate problem like this. 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
On 12/23/07, OGAWA Hirofumi <[EMAIL PROTECTED]> wrote: > "Grant Likely" <[EMAIL PROTECTED]> writes: > > > > 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). I not sure I fully understand what you mean. Can you please elaborate? Are you referring to whether on not it will break other FAT implementations if a file has more clusters allocated than it needs? If so, how do we decide whether or not it is acceptable? Thanks, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 -- 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
"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(>i_mutex); >> + ret = fat_cont_expand(inode, (offset + len)); >> + if (ret) { >> + printk(KERN_ERR >> + "fat_fallocate():fat_cont_expand() error\n"); >> + mutex_unlock(>i_mutex); >> + return ret; >> + } >> + mutex_unlock(>i_mutex); >> + } >> + if (mode & FALLOC_FL_KEEP_SIZE) { >> + mutex_lock(>i_mutex); >> + i_size_write(inode, filesize); >> + mutex_unlock(>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/
Re: [PATCH] fat: Editions to support fat_fallocate()
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/
Re: [PATCH] fat: Editions to support fat_fallocate()
On 12/23/07, OGAWA Hirofumi [EMAIL PROTECTED] wrote: Grant Likely [EMAIL PROTECTED] writes: 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). I not sure I fully understand what you mean. Can you please elaborate? Are you referring to whether on not it will break other FAT implementations if a file has more clusters allocated than it needs? If so, how do we decide whether or not it is acceptable? Thanks, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 -- 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
Thanks Steve, comments below. On 12/22/07, Steven Cavanagh <[EMAIL PROTECTED]> wrote: > From: Steven Cavanagh <[EMAIL PROTECTED]> > > Added support for fallocate for a msdos fat driver. This allows > preallocation of clusters to an inode before writes to reduce > file fragmentation Not technically true. This doesn't make any guarantees about fragmentation (even if in general it works pretty well). To really reduce fragmentation, then cluster allocation needs to be made smarter so it goes looking for contiguous clusters when allocating, but that's a task for a separate patch. Please adjust your description. Also, please briefly describe the testing that you've performed. More comments below. > Signed-off-by: Steven.Cavanagh <[EMAIL PROTECTED]> > --- > > fs/fat/file.c | 45 + > 1 files changed, 45 insertions(+), 0 deletions(-) > > diff --git a/fs/fat/file.c b/fs/fat/file.c > index 69a83b5..f753c6a 100644 > --- a/fs/fat/file.c > +++ b/fs/fat/file.c > @@ -15,6 +15,7 @@ #include > #include > #include > #include > +#include > > int fat_generic_ioctl(struct inode *inode, struct file *filp, > unsigned int cmd, unsigned long arg) > @@ -312,8 +313,52 @@ int fat_getattr(struct vfsmount *mnt, st > } > EXPORT_SYMBOL_GPL(fat_getattr); > > +/* > + * 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) > +{ > + 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() > + 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. > + return 0; > + } > + > + if ((offset + len) > MSDOS_I(inode)->mmu_private) { > + > + mutex_lock(>i_mutex); > + ret = fat_cont_expand(inode, (offset + len)); > + if (ret) { > + printk(KERN_ERR > + "fat_fallocate():fat_cont_expand() error\n"); > + mutex_unlock(>i_mutex); > + return ret; > + } > + mutex_unlock(>i_mutex); > + } > + if (mode & FALLOC_FL_KEEP_SIZE) { > + mutex_lock(>i_mutex); > + i_size_write(inode, filesize); > + mutex_unlock(>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. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 -- 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
Thanks Steve, comments below. On 12/22/07, Steven Cavanagh [EMAIL PROTECTED] wrote: From: Steven Cavanagh [EMAIL PROTECTED] Added support for fallocate for a msdos fat driver. This allows preallocation of clusters to an inode before writes to reduce file fragmentation Not technically true. This doesn't make any guarantees about fragmentation (even if in general it works pretty well). To really reduce fragmentation, then cluster allocation needs to be made smarter so it goes looking for contiguous clusters when allocating, but that's a task for a separate patch. Please adjust your description. Also, please briefly describe the testing that you've performed. More comments below. Signed-off-by: Steven.Cavanagh [EMAIL PROTECTED] --- fs/fat/file.c | 45 + 1 files changed, 45 insertions(+), 0 deletions(-) diff --git a/fs/fat/file.c b/fs/fat/file.c index 69a83b5..f753c6a 100644 --- a/fs/fat/file.c +++ b/fs/fat/file.c @@ -15,6 +15,7 @@ #include linux/buffer_head.h #include linux/writeback.h #include linux/backing-dev.h #include linux/blkdev.h +#include linux/falloc.h int fat_generic_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg) @@ -312,8 +313,52 @@ int fat_getattr(struct vfsmount *mnt, st } EXPORT_SYMBOL_GPL(fat_getattr); +/* + * 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) +{ + 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() + 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. + return 0; + } + + if ((offset + len) MSDOS_I(inode)-mmu_private) { + + 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. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 -- 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
Thanks Steve; I've cc'd LKML and Hirofumi in my reply. Cheers, g. On 12/14/07, Steven Cavanagh <[EMAIL PROTECTED]> wrote: > From: Steven Cavanagh <[EMAIL PROTECTED]> > > Added support for fallocate for a msdos fat driver. This allows > preallocation of clusters to an inode before writes to reduce > file fragmentation > > Signed-off-by: Steven.Cavanagh <[EMAIL PROTECTED]> > --- > > fs/fat/cache.c |9 + > fs/fat/file.c | 47 +++ > fs/fat/inode.c | 15 +++ > 3 files changed, 71 insertions(+), 0 deletions(-) > > diff --git a/fs/fat/cache.c b/fs/fat/cache.c > index 639b3b4..1a69ce4 100644 > --- a/fs/fat/cache.c > +++ b/fs/fat/cache.c Drop your changes to this file; they are just pr_debug statements that don't need to be in mainline. > diff --git a/fs/fat/file.c b/fs/fat/file.c > index 69a83b5..de3f9ee 100644 > --- a/fs/fat/file.c > +++ b/fs/fat/file.c > @@ -6,6 +6,8 @@ > * regular file handling primitives for fat-based filesystems > */ > > +#undef DEBUG > + Drop these 2 lines > #include > #include > #include > @@ -15,6 +17,7 @@ #include > #include > #include > #include > +#include > > int fat_generic_ioctl(struct inode *inode, struct file *filp, > unsigned int cmd, unsigned long arg) > @@ -312,8 +315,52 @@ int fat_getattr(struct vfsmount *mnt, st > } > EXPORT_SYMBOL_GPL(fat_getattr); > > +/* > + * 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) > +{ > + 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"); > + return -ENODEV; > + } > + > + if ((offset + len) <= filesize) { > + printk(KERN_ERR > + "fat_fallocate():Blocks already allocated\n"); > + return -EIO; > + } Drop the printk... in fact, dorp the error code too and just return 0. It's not an IO error if the space has already been allocated. In fact, this test is probably irrelevant. Since we're allocating clusters and not necessarily increasing the file size, you should instead test to see if the requested region already has clusters allocated. > + if (offset > filesize) { > + printk(KERN_ERR > + "fat_fallocate():Offset error\n"); > + return -EIO; > + } I though we agreed that we *want* to support this case. ie. if the caller specifies an offset beyond the length of the file, then allocate clusters to cover both the requested region and the 'gap'. > + > + if ((offset + len) > filesize) { Again, you don't want to test against the filesize; you want to test against the number of allocated sectors. > + pr_debug("fat_fallocate():fat_cont_expand(): size: %llu\n", > + (offset+len)); > + ret = fat_cont_expand(inode, (offset + len)); > + } What if fat_cont_expand fails? You'll end up increasing the filesize regardless. > + if (mode & FALLOC_FL_KEEP_SIZE) { > + mutex_lock(>i_mutex); The lock/unlock needs to also protect fat_cont_expand. > + i_size_write(inode, filesize); > + mutex_unlock(>i_mutex); > + pr_debug( > + "fat_fallocate():FALLOC_FL_KEEP_SIZE: %llu\n", filesize); > + } > + return ret; > +} > + > const struct inode_operations fat_file_inode_operations = { > .truncate = fat_truncate, > .setattr= fat_notify_change, > .getattr= fat_getattr, > + .fallocate = fat_fallocate, > }; > diff --git a/fs/fat/inode.c b/fs/fat/inode.c > index 920a576..ad6f069 100644 > --- a/fs/fat/inode.c > +++ b/fs/fat/inode.c Same for this file; this is just the addition of pr_debugs; drop from this patch -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 -- 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/
Re: [PATCH] fat: Editions to support fat_fallocate()
Thanks Steve; I've cc'd LKML and Hirofumi in my reply. Cheers, g. On 12/14/07, Steven Cavanagh [EMAIL PROTECTED] wrote: From: Steven Cavanagh [EMAIL PROTECTED] Added support for fallocate for a msdos fat driver. This allows preallocation of clusters to an inode before writes to reduce file fragmentation Signed-off-by: Steven.Cavanagh [EMAIL PROTECTED] --- fs/fat/cache.c |9 + fs/fat/file.c | 47 +++ fs/fat/inode.c | 15 +++ 3 files changed, 71 insertions(+), 0 deletions(-) diff --git a/fs/fat/cache.c b/fs/fat/cache.c index 639b3b4..1a69ce4 100644 --- a/fs/fat/cache.c +++ b/fs/fat/cache.c Drop your changes to this file; they are just pr_debug statements that don't need to be in mainline. snip diff --git a/fs/fat/file.c b/fs/fat/file.c index 69a83b5..de3f9ee 100644 --- a/fs/fat/file.c +++ b/fs/fat/file.c @@ -6,6 +6,8 @@ * regular file handling primitives for fat-based filesystems */ +#undef DEBUG + Drop these 2 lines #include linux/capability.h #include linux/module.h #include linux/time.h @@ -15,6 +17,7 @@ #include linux/buffer_head.h #include linux/writeback.h #include linux/backing-dev.h #include linux/blkdev.h +#include linux/falloc.h int fat_generic_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg) @@ -312,8 +315,52 @@ int fat_getattr(struct vfsmount *mnt, st } EXPORT_SYMBOL_GPL(fat_getattr); +/* + * 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) +{ + 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); + return -ENODEV; + } + + if ((offset + len) = filesize) { + printk(KERN_ERR + fat_fallocate():Blocks already allocated\n); + return -EIO; + } Drop the printk... in fact, dorp the error code too and just return 0. It's not an IO error if the space has already been allocated. In fact, this test is probably irrelevant. Since we're allocating clusters and not necessarily increasing the file size, you should instead test to see if the requested region already has clusters allocated. + if (offset filesize) { + printk(KERN_ERR + fat_fallocate():Offset error\n); + return -EIO; + } I though we agreed that we *want* to support this case. ie. if the caller specifies an offset beyond the length of the file, then allocate clusters to cover both the requested region and the 'gap'. + + if ((offset + len) filesize) { Again, you don't want to test against the filesize; you want to test against the number of allocated sectors. + pr_debug(fat_fallocate():fat_cont_expand(): size: %llu\n, + (offset+len)); + ret = fat_cont_expand(inode, (offset + len)); + } What if fat_cont_expand fails? You'll end up increasing the filesize regardless. + if (mode FALLOC_FL_KEEP_SIZE) { + mutex_lock(inode-i_mutex); The lock/unlock needs to also protect fat_cont_expand. + i_size_write(inode, filesize); + mutex_unlock(inode-i_mutex); + pr_debug( + fat_fallocate():FALLOC_FL_KEEP_SIZE: %llu\n, filesize); + } + return ret; +} + const struct inode_operations fat_file_inode_operations = { .truncate = fat_truncate, .setattr= fat_notify_change, .getattr= fat_getattr, + .fallocate = fat_fallocate, }; diff --git a/fs/fat/inode.c b/fs/fat/inode.c index 920a576..ad6f069 100644 --- a/fs/fat/inode.c +++ b/fs/fat/inode.c Same for this file; this is just the addition of pr_debugs; drop from this patch snip -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 -- 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/