2005-09-14 20:36:56

by Machida, Hiroyuki

[permalink] [raw]
Subject: [PATCH 1/2][FAT] miss-sync issues on sync mount (miss-sync on write)

Signed-off-by: Hiroyuki Machida <[email protected]>
---

file.c | 20 +-------------------
inode.c | 21 ++++++++++++++++++++-
2 files changed, 21 insertions(+), 20 deletions(-)


--- linux-2.6.13.org/fs/fat/inode.c 2005-08-29 08:41:01.000000000 +0900
+++ linux-2.6.13/fs/fat/inode.c 2005-09-09 17:22:44.305189111 +0900
@@ -107,12 +107,31 @@ static sector_t _fat_bmap(struct address
return generic_block_bmap(mapping, block, fat_get_block);
}

+static int fat_commit_write(struct file *file, struct page *page,
+ unsigned from, unsigned to)
+{
+ struct inode *inode = page->mapping->host;
+ loff_t pos = ((loff_t)page->index << PAGE_CACHE_SHIFT) + to;
+ block_commit_write(page,from,to);
+ /*
+ * No need to use i_size_read() here, the i_size
+ * cannot change under us because we hold i_sem.
+ */
+ if (pos > inode->i_size) {
+ i_size_write(inode, pos);
+ }
+ inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
+ MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
+ mark_inode_dirty(inode);
+ return 0;
+}
+
static struct address_space_operations fat_aops = {
.readpage = fat_readpage,
.writepage = fat_writepage,
.sync_page = block_sync_page,
.prepare_write = fat_prepare_write,
- .commit_write = generic_commit_write,
+ .commit_write = fat_commit_write,
.bmap = _fat_bmap
};

--- linux-2.6.13.org/fs/fat/file.c 2005-08-29 08:41:01.000000000 +0900
+++ linux-2.6.13/fs/fat/file.c 2005-09-08 17:13:03.000000000 +0900
@@ -12,24 +12,6 @@
#include <linux/smp_lock.h>
#include <linux/buffer_head.h>

-static ssize_t fat_file_aio_write(struct kiocb *iocb, const char __user *buf,
- size_t count, loff_t pos)
-{
- struct inode *inode = iocb->ki_filp->f_dentry->d_inode;
- int retval;
-
- retval = generic_file_aio_write(iocb, buf, count, pos);
- if (retval > 0) {
- inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
- MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
- mark_inode_dirty(inode);
-// check the locking rules
-// if (IS_SYNC(inode))
-// fat_sync_inode(inode);
- }
- return retval;
-}
-
static ssize_t fat_file_writev(struct file *filp, const struct iovec *iov,
unsigned long nr_segs, loff_t *ppos)
{
@@ -150,7 +132,7 @@ struct file_operations fat_file_operatio
.readv = generic_file_readv,
.writev = fat_file_writev,
.aio_read = generic_file_aio_read,
- .aio_write = fat_file_aio_write,
+ .aio_write = generic_file_aio_write,
.mmap = generic_file_mmap,
.ioctl = fat_generic_ioctl,
.fsync = file_fsync,


Attachments:
fat-sync-write.patch (2.41 kB)

2005-09-15 02:48:56

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 1/2][FAT] miss-sync issues on sync mount (miss-sync on write)

"Machida, Hiroyuki" <[email protected]> wrote:
>
> This patch fixes miss-sync issue on write() system call.
> This updates inode attrs flags, mtime and ctime on every
> comit_write call, due to locking.

This all seems wrong.

Why does fatfs have file_operations.write pointing at do_sync_write()
rather than generic_file_write()?

Why does fatfs have a custom .aio_write() rather than using
generic_file_aio_write()?

If fatfs can use all the standard library functions, all this inode
dirtying and O_SYNC/-o sync handling shoud just work.

2005-09-15 12:43:31

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH 1/2][FAT] miss-sync issues on sync mount (miss-sync on write)

Andrew Morton <[email protected]> writes:

> "Machida, Hiroyuki" <[email protected]> wrote:
>>
>> This patch fixes miss-sync issue on write() system call.
>> This updates inode attrs flags, mtime and ctime on every
>> comit_write call, due to locking.
>
> This all seems wrong.
>
> Why does fatfs have file_operations.write pointing at do_sync_write()
> rather than generic_file_write()?
>
> Why does fatfs have a custom .aio_write() rather than using
> generic_file_aio_write()?
>
> If fatfs can use all the standard library functions, all this inode
> dirtying and O_SYNC/-o sync handling shoud just work.

In fat_file_aio_write:

retval = generic_file_aio_write(iocb, buf, count, pos);
if (retval > 0) {
inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
mark_inode_dirty(inode);

He says about the above FAT specific flag and timestamp. FAT needs to
add the ATTR_ARCH if data was modified, but current FAT is adding it
after generic_file_aio_write()/generic_file_writev(). (IIRC, some
other filesystems is also doing this.)

His patch seems good, although the some additional works was needed.
I applied the attached patch. Thanks, Hiroyuki-san.

Andrew, now fat_file_operations is the following. Is this OK?

struct file_operations fat_file_operations = {
.llseek = generic_file_llseek,
.read = do_sync_read,
.write = do_sync_write,
.readv = generic_file_readv,
.writev = generic_file_writev,
.aio_read = generic_file_aio_read,
.aio_write = generic_file_aio_write,
.mmap = generic_file_mmap,
.ioctl = fat_generic_ioctl,
.fsync = file_fsync,
.sendfile = generic_file_sendfile,
};
--
OGAWA Hirofumi <[email protected]>



[PATCH] FAT: miss-sync issues on sync mount (miss-sync on write)

This patch fixes miss-sync issue on write() system call.
This updates inode attrs flags, mtime and ctime on every
comit_write call, due to locking.

Signed-off-by: Hiroyuki Machida <[email protected]>
Signed-off-by: OGAWA Hirofumi <[email protected]>
---

fs/fat/file.c | 37 ++-----------------------------------
fs/fat/inode.c | 15 ++++++++++++++-
2 files changed, 16 insertions(+), 36 deletions(-)

diff -puN fs/fat/file.c~fat_arch-flags-fix fs/fat/file.c
--- linux-2.6.14-rc1/fs/fat/file.c~fat_arch-flags-fix 2005-09-15 20:52:21.000000000 +0900
+++ linux-2.6.14-rc1-hirofumi/fs/fat/file.c 2005-09-15 20:55:12.000000000 +0900
@@ -12,39 +12,6 @@
#include <linux/smp_lock.h>
#include <linux/buffer_head.h>

-static ssize_t fat_file_aio_write(struct kiocb *iocb, const char __user *buf,
- size_t count, loff_t pos)
-{
- struct inode *inode = iocb->ki_filp->f_dentry->d_inode;
- int retval;
-
- retval = generic_file_aio_write(iocb, buf, count, pos);
- if (retval > 0) {
- inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
- MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
- mark_inode_dirty(inode);
-// check the locking rules
-// if (IS_SYNC(inode))
-// fat_sync_inode(inode);
- }
- return retval;
-}
-
-static ssize_t fat_file_writev(struct file *filp, const struct iovec *iov,
- unsigned long nr_segs, loff_t *ppos)
-{
- struct inode *inode = filp->f_dentry->d_inode;
- int retval;
-
- retval = generic_file_writev(filp, iov, nr_segs, ppos);
- if (retval > 0) {
- inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
- MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
- mark_inode_dirty(inode);
- }
- return retval;
-}
-
int fat_generic_ioctl(struct inode *inode, struct file *filp,
unsigned int cmd, unsigned long arg)
{
@@ -148,9 +115,9 @@ struct file_operations fat_file_operatio
.read = do_sync_read,
.write = do_sync_write,
.readv = generic_file_readv,
- .writev = fat_file_writev,
+ .writev = generic_file_writev,
.aio_read = generic_file_aio_read,
- .aio_write = fat_file_aio_write,
+ .aio_write = generic_file_aio_write,
.mmap = generic_file_mmap,
.ioctl = fat_generic_ioctl,
.fsync = file_fsync,
diff -puN fs/fat/inode.c~fat_arch-flags-fix fs/fat/inode.c
--- linux-2.6.14-rc1/fs/fat/inode.c~fat_arch-flags-fix 2005-09-15 20:52:21.000000000 +0900
+++ linux-2.6.14-rc1-hirofumi/fs/fat/inode.c 2005-09-15 21:32:51.000000000 +0900
@@ -102,6 +102,19 @@ static int fat_prepare_write(struct file
&MSDOS_I(page->mapping->host)->mmu_private);
}

+static int fat_commit_write(struct file *file, struct page *page,
+ unsigned from, unsigned to)
+{
+ struct inode *inode = page->mapping->host;
+ int err = generic_commit_write(file, page, from, to);
+ if (!err && !(MSDOS_I(inode)->i_attrs & ATTR_ARCH)) {
+ inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
+ MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
+ mark_inode_dirty(inode);
+ }
+ return err;
+}
+
static sector_t _fat_bmap(struct address_space *mapping, sector_t block)
{
return generic_block_bmap(mapping, block, fat_get_block);
@@ -112,7 +125,7 @@ static struct address_space_operations f
.writepage = fat_writepage,
.sync_page = block_sync_page,
.prepare_write = fat_prepare_write,
- .commit_write = generic_commit_write,
+ .commit_write = fat_commit_write,
.bmap = _fat_bmap
};

_