2007-12-12 02:55:51

by Steven Cavanagh

[permalink] [raw]
Subject: [PATCH] fat: Editions to support fat_fallocate()

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 */
+ mapped_blocks = sbi->sec_per_clus - cluster_offset;
+ pr_debug("msdos_fallocate():mapped_blocks:%lu\n",
+ mapped_blocks);
+
+ /*mapped_blocks and dos_max_blocks should
+ *be the same because mapped_blocks used to
+ *be calculated in __fat_get_block() as
+ * sbi->sec_per_clus - offset(sector offset)
+ */
+ max_blocks = min(mapped_blocks, (new_blocks-nblocks));
+
+ MSDOS_I(inode)->mmu_private +=
+ max_blocks << sb->s_blocksize_bits;
+
+ pr_debug("msdos_fallocate():MSDOS_I(inode)->mmu_private:%llu\n",
+ MSDOS_I(inode)->mmu_private);
+
+ pr_debug("msdos_fallocate():mapped_blocks:%lu,max_blocks:%lu\n",
+ mapped_blocks, max_blocks);
+
+ err = fat_bmap(inode, offset_block, &phys, &mapped_blocks);
+ if (err) {
+ printk(KERN_ERR
+ "msdos_fallocate():fat_bmap() error:%d\n", err);
+ return err;
+ }
+ pr_debug("msdos_fallocate():sector: %lu, phys: %llu\n",
+ offset_block, phys);
+
+ if (!phys) {
+ printk(KERN_ERR
+ "msdos_fallocate():Bad disk sector number\n");
+ return -EIO;
+ }
+ if (!mapped_blocks) {
+ printk(KERN_ERR
+ "msdos_fallocate():Block mapping error\n");
+ return -EIO;
+ }
+ set_buffer_new(&bh);
+ map_bh(&bh, sb, phys);
+ pr_debug("msdos_fallocate():b_size:%d, b_blocknr:%llu\n",
+ bh.b_size, bh.b_blocknr);
+
+ now = current_fs_time(inode->i_sb);
+ if (!timespec_equal(&inode->i_ctime, &now))
+ inode->i_ctime = now;
+
+ /*Increment the cluster block count*/
+ nblocks += mapped_blocks;
+ offset_block += mapped_blocks;
+ }
+
+ pr_debug("msdos_fallocate():fat_bmap():nblocks:%lu\n",
+ nblocks);
+ mark_inode_dirty(inode);
+
+ /*
+ * Time to update the file size.
+ * Update only when preallocation was requested beyond the file size.
+ */
+ if (!(mode & FALLOC_FL_KEEP_SIZE) &&
+ (offset + len) > i_size_read(inode)) {
+ if (!ret) {
+ /*
+ * if no error, we assume preallocation succeeded
+ * completely
+ */
+ mutex_lock(&inode->i_mutex);
+ i_size_write(inode, offset + len);
+ mutex_unlock(&inode->i_mutex);
+ pr_debug("msdos_fallocate():INODE SIZE:%llu\n",
+ i_size_read(inode));
+
+ } else if (ret && nblocks) {
+
+ printk(KERN_ERR
+ "msdos_fallocate(): Errors, updating inode size\n");
+ /* Handle partial allocation scenario */
+ mutex_lock(&inode->i_mutex);
+ newsize = (nblocks << blkbits) + i_size_read(inode);
+ i_size_write(inode,
+ MSDOS_BLOCK_ALIGN(newsize, blkbits));
+ mutex_unlock(&inode->i_mutex);
+ pr_debug("msdos_fallocate():INODE SIZE:%llu\n",
+ i_size_read(inode));
+ }
+ }
+
+ 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..463ead5 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -10,6 +10,8 @@
* Max Cohan: Fixed invalid FSINFO offset when info_sector is 0
*/

+#undef DEBUG
+
#include <linux/module.h>
#include <linux/init.h>
#include <linux/time.h>
@@ -38,7 +40,7 @@ static int fat_default_codepage = CONFIG
static char fat_default_iocharset[] = CONFIG_FAT_DEFAULT_IOCHARSET;


-static int fat_add_cluster(struct inode *inode)
+int fat_add_cluster(struct inode *inode)
{
int err, cluster;

@@ -64,6 +66,11 @@ static inline int __fat_get_block(struct
int err, offset;

err = fat_bmap(inode, iblock, &phys, &mapped_blocks);
+
+ pr_debug(
+ "__fat_get_block():phys: %llu, mapped_blocks:%lu,sector: %llu\n",
+ phys, mapped_blocks, iblock);
+
if (err)
return err;
if (phys) {
@@ -90,10 +97,18 @@ static inline int __fat_get_block(struct
/* available blocks on this cluster */
mapped_blocks = sbi->sec_per_clus - offset;

+ pr_debug(
+ "__fat_get_block(): max_blocks: %lu, mapped_blocks:%lu\n",
+ *max_blocks, mapped_blocks);
+
*max_blocks = min(mapped_blocks, *max_blocks);
MSDOS_I(inode)->mmu_private += *max_blocks << sb->s_blocksize_bits;

err = fat_bmap(inode, iblock, &phys, &mapped_blocks);
+ pr_debug(
+ "__fat_get_block():phys: %llu, mapped_blocks:%lu,sector: %llu\n",
+ phys, mapped_blocks, iblock);
+
if (err)
return err;

@@ -116,6 +131,10 @@ static int fat_get_block(struct inode *i
if (err)
return err;
bh_result->b_size = max_blocks << sb->s_blocksize_bits;
+
+ printk(KERN_INFO "fat_get_block():bh_result->b_size:%d\n",
+ bh_result->b_size);
+
return 0;
}

diff --git a/include/linux/msdos_fs.h b/include/linux/msdos_fs.h
index f950921..034a2a4 100644
--- a/include/linux/msdos_fs.h
+++ b/include/linux/msdos_fs.h
@@ -109,6 +109,8 @@ #define VFAT_SFN_DISPLAY_WINNT 0x0004 /*
#define VFAT_SFN_CREATE_WIN95 0x0100 /* emulate win95 rule for create */
#define VFAT_SFN_CREATE_WINNT 0x0200 /* emulate winnt rule for create */

+#define MSDOS_BLOCK_ALIGN(size, blkbits) ALIGN((size), (1 << (blkbits)))
+
struct fat_boot_sector {
__u8 ignored[3]; /* Boot strap short or near jump */
__u8 system_id[8]; /* Name - can be used to special case
@@ -350,6 +352,9 @@ extern int fat_add_entries(struct inode
struct fat_slot_info *sinfo);
extern int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo);

+/* fat/inode.c */
+extern int fat_add_cluster(struct inode *inode);
+
/* fat/fatent.c */
struct fat_entry {
int entry;


2007-12-14 20:13:04

by Grant Likely

[permalink] [raw]
Subject: 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

2007-12-22 22:18:32

by Grant Likely

[permalink] [raw]
Subject: 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

2007-12-23 12:16:34

by OGAWA Hirofumi

[permalink] [raw]
Subject: 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]>

2007-12-23 20:23:33

by Grant Likely

[permalink] [raw]
Subject: 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

2008-01-13 19:50:00

by OGAWA Hirofumi

[permalink] [raw]
Subject: 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]>