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

[OGAWA Hirofumi]

"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()

[OGAWA Hirofumi]

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]

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()

[OGAWA Hirofumi]

"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()

[OGAWA Hirofumi]

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()

[Grant Likely]

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/

[PATCH] fat: Editions to support fat_fallocate()

[Steven Cavanagh]

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/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");
+   return -ENODEV;
+   }
+
+   if ((offset + len) <= MSDOS_I(inode)->mmu_private) {
+   printk(KERN_INFO
+   "fat_fallocate():Blocks already allocated\n");
+   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);
+   }
+   return ret;
+}
+
 const struct inode_operations fat_file_inode_operations = {
.truncate   = fat_truncate,
.setattr= fat_notify_change,
.getattr= fat_getattr,
+   .fallocate  = fat_fallocate,
 };
--
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]

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()

[Grant Likely]

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/

[PATCH] fat: Editions to support fat_fallocate()

[Steven Cavanagh]

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/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);
+   return -ENODEV;
+   }
+
+   if ((offset + len) = MSDOS_I(inode)-mmu_private) {
+   printk(KERN_INFO
+   fat_fallocate():Blocks already allocated\n);
+   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);
+   }
+   return ret;
+}
+
 const struct inode_operations fat_file_inode_operations = {
.truncate   = fat_truncate,
.setattr= fat_notify_change,
.getattr= fat_getattr,
+   .fallocate  = fat_fallocate,
 };
--
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]

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()

[Grant Likely]

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/

[PATCH] fat: Editions to support fat_fallocate()

[Steven Cavanagh]

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|  167 ++
 fs/fat/inode.c   |   21 ++
 include/linux/msdos_fs.h |5 +
 4 files changed, 201 insertions(+), 1 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
@@ -8,6 +8,8 @@
  *  May 1999. AV. Fixed the bogosity with FAT32 (read "FAT28"). Fscking lusers.
  */
 
+#undef DEBUG
+
 #include 
 #include 
 #include 
@@ -316,6 +318,10 @@ int fat_bmap(struct inode *inode, sector
 
cluster = sector >> (sbi->cluster_bits - sb->s_blocksize_bits);
offset  = sector & (sbi->sec_per_clus - 1);
+
+   pr_debug("fat_bmap():cluster:%d, offset:%d, last_block:%llu\n",
+   cluster, offset, last_block);
+
cluster = fat_bmap_cluster(inode, cluster);
if (cluster < 0)
return cluster;
@@ -324,6 +330,9 @@ int fat_bmap(struct inode *inode, sector
*mapped_blocks = sbi->sec_per_clus - offset;
if (*mapped_blocks > last_block - sector)
*mapped_blocks = last_block - sector;
+
+   pr_debug("fat_bmap():cluster:%d, phys:%llu\n",
+   cluster, *phys);
}
return 0;
 }
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 69a83b5..9698d42 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
+
 #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,172 @@ 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)
+{
+   unsigned int blkbits = inode->i_blkbits;
+   int ret = 0, err;
+   unsigned long offset_block, new_blocks;
+   unsigned long max_blocks, nblocks = 0;
+   unsigned long mapped_blocks = 0, cluster_offset = 0;
+
+   struct buffer_head bh;
+
+   loff_t newsize = 0;
+
+   struct super_block *sb = inode->i_sb;
+   struct msdos_sb_info *sbi = MSDOS_SB(sb);
+   sector_t phys;
+   struct timespec now;
+
+   /* 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;
+   }
+
+   offset_block = offset >> blkbits;
+   pr_debug("fat_fallocate:offset block:%lu\n", offset_block);
+
+   /* Determine new allocation block */
+   new_blocks = (MSDOS_BLOCK_ALIGN(len + offset, blkbits) >> blkbits)
+   - offset_block;
+   pr_debug("fat_fallocate:allocate block:%lu\n", new_blocks);
+
+   if ((offset_block + new_blocks) <=
+   (MSDOS_BLOCK_ALIGN(i_size_read(inode), blkbits)
+   >> blkbits)) {
+   printk(KERN_ERR
+   "fat_fallocate():Blocks already allocated\n");
+   return -EIO;
+   }
+   if (offset_block >
+   (MSDOS_BLOCK_ALIGN(i_size_read(inode), blkbits)
+   >> blkbits)) {
+   printk(KERN_ERR
+   "fat_fallocate():Offset error\n");
+   return -EIO;
+   }
+   while (ret >= 0 && nblocks < new_blocks) {
+
+   /* Allocate a new cluster after all the available
+* blocks(sectors) have been allocated.
+*/
+   cluster_offset =
+   (unsigned long)offset_block & (sbi->sec_per_clus - 1);
+   if (!cluster_offset) {
+
+   ret = fat_add_cluster(inode);
+   if (ret) {
+   pr_debug("msdos_fallocate():Add cluster err\
+   inode#%lu, block = %lu, max_blocks = %lu\n",
+   inode->i_ino, offset_block, new_blocks);
+   break;
+   }
+   }
+
+   /* mapped blocks = 4 blocks/sector - offset into cluster */
+   mapped_blocks = sbi->sec_per_clus - cluster_offset;
+   pr_debug("msdos_fallocate():mapped_blocks:%lu\n",
+  

[PATCH] fat: Editions to support fat_fallocate()

[Steven Cavanagh]

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|  167 ++
 fs/fat/inode.c   |   21 ++
 include/linux/msdos_fs.h |5 +
 4 files changed, 201 insertions(+), 1 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
@@ -8,6 +8,8 @@
  *  May 1999. AV. Fixed the bogosity with FAT32 (read FAT28). Fscking lusers.
  */
 
+#undef DEBUG
+
 #include linux/fs.h
 #include linux/msdos_fs.h
 #include linux/buffer_head.h
@@ -316,6 +318,10 @@ int fat_bmap(struct inode *inode, sector
 
cluster = sector  (sbi-cluster_bits - sb-s_blocksize_bits);
offset  = sector  (sbi-sec_per_clus - 1);
+
+   pr_debug(fat_bmap():cluster:%d, offset:%d, last_block:%llu\n,
+   cluster, offset, last_block);
+
cluster = fat_bmap_cluster(inode, cluster);
if (cluster  0)
return cluster;
@@ -324,6 +330,9 @@ int fat_bmap(struct inode *inode, sector
*mapped_blocks = sbi-sec_per_clus - offset;
if (*mapped_blocks  last_block - sector)
*mapped_blocks = last_block - sector;
+
+   pr_debug(fat_bmap():cluster:%d, phys:%llu\n,
+   cluster, *phys);
}
return 0;
 }
diff --git a/fs/fat/file.c b/fs/fat/file.c
index 69a83b5..9698d42 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
+
 #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,172 @@ 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)
+{
+   unsigned int blkbits = inode-i_blkbits;
+   int ret = 0, err;
+   unsigned long offset_block, new_blocks;
+   unsigned long max_blocks, nblocks = 0;
+   unsigned long mapped_blocks = 0, cluster_offset = 0;
+
+   struct buffer_head bh;
+
+   loff_t newsize = 0;
+
+   struct super_block *sb = inode-i_sb;
+   struct msdos_sb_info *sbi = MSDOS_SB(sb);
+   sector_t phys;
+   struct timespec now;
+
+   /* 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;
+   }
+
+   offset_block = offset  blkbits;
+   pr_debug(fat_fallocate:offset block:%lu\n, offset_block);
+
+   /* Determine new allocation block */
+   new_blocks = (MSDOS_BLOCK_ALIGN(len + offset, blkbits)  blkbits)
+   - offset_block;
+   pr_debug(fat_fallocate:allocate block:%lu\n, new_blocks);
+
+   if ((offset_block + new_blocks) =
+   (MSDOS_BLOCK_ALIGN(i_size_read(inode), blkbits)
+blkbits)) {
+   printk(KERN_ERR
+   fat_fallocate():Blocks already allocated\n);
+   return -EIO;
+   }
+   if (offset_block 
+   (MSDOS_BLOCK_ALIGN(i_size_read(inode), blkbits)
+blkbits)) {
+   printk(KERN_ERR
+   fat_fallocate():Offset error\n);
+   return -EIO;
+   }
+   while (ret = 0  nblocks  new_blocks) {
+
+   /* Allocate a new cluster after all the available
+* blocks(sectors) have been allocated.
+*/
+   cluster_offset =
+   (unsigned long)offset_block  (sbi-sec_per_clus - 1);
+   if (!cluster_offset) {
+
+   ret = fat_add_cluster(inode);
+   if (ret) {
+   pr_debug(msdos_fallocate():Add cluster err\
+   inode#%lu, block = %lu, max_blocks = %lu\n,
+   inode-i_ino, offset_block, new_blocks);
+   break;
+   }
+   }
+
+   /* mapped blocks = 4 blocks/sector - offset into cluster */
+