* [PATCH 01/11] fuse: Linking file to inode helper
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
@ 2013-10-10 13:10 ` Maxim Patlasov
2013-10-10 13:10 ` [PATCH 02/11] fuse: Prepare to handle short reads Maxim Patlasov
` (10 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:10 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
When writeback is ON every writeable file should be in per-inode write list,
not only mmap-ed ones. Thus introduce a helper for this linkage.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
fs/fuse/file.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 3f219fd..5e49c8f 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -171,6 +171,22 @@ int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
}
EXPORT_SYMBOL_GPL(fuse_do_open);
+static void fuse_link_write_file(struct file *file)
+{
+ struct inode *inode = file_inode(file);
+ struct fuse_conn *fc = get_fuse_conn(inode);
+ struct fuse_inode *fi = get_fuse_inode(inode);
+ struct fuse_file *ff = file->private_data;
+ /*
+ * file may be written through mmap, so chain it onto the
+ * inodes's write_file list
+ */
+ spin_lock(&fc->lock);
+ if (list_empty(&ff->write_entry))
+ list_add(&ff->write_entry, &fi->write_files);
+ spin_unlock(&fc->lock);
+}
+
void fuse_finish_open(struct inode *inode, struct file *file)
{
struct fuse_file *ff = file->private_data;
@@ -1929,20 +1945,9 @@ static const struct vm_operations_struct fuse_file_vm_ops = {
static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma)
{
- if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_MAYWRITE)) {
- struct inode *inode = file_inode(file);
- struct fuse_conn *fc = get_fuse_conn(inode);
- struct fuse_inode *fi = get_fuse_inode(inode);
- struct fuse_file *ff = file->private_data;
- /*
- * file may be written through mmap, so chain it onto the
- * inodes's write_file list
- */
- spin_lock(&fc->lock);
- if (list_empty(&ff->write_entry))
- list_add(&ff->write_entry, &fi->write_files);
- spin_unlock(&fc->lock);
- }
+ if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_MAYWRITE))
+ fuse_link_write_file(file);
+
file_accessed(file);
vma->vm_ops = &fuse_file_vm_ops;
return 0;
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 02/11] fuse: Prepare to handle short reads
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
2013-10-10 13:10 ` [PATCH 01/11] fuse: Linking file to inode helper Maxim Patlasov
@ 2013-10-10 13:10 ` Maxim Patlasov
2013-10-10 13:10 ` [PATCH 03/11] fuse: Connection bit for enabling writeback Maxim Patlasov
` (9 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:10 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
A helper which gets called when read reports less bytes than was requested.
See patch #4 (trust kernel i_size only) for details.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
fs/fuse/file.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 5e49c8f..886f3a2 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -654,6 +654,15 @@ static void fuse_read_update_size(struct inode *inode, loff_t size,
spin_unlock(&fc->lock);
}
+static void fuse_short_read(struct fuse_req *req, struct inode *inode,
+ u64 attr_ver)
+{
+ size_t num_read = req->out.args[0].size;
+
+ loff_t pos = page_offset(req->pages[0]) + num_read;
+ fuse_read_update_size(inode, pos, attr_ver);
+}
+
static int fuse_readpage(struct file *file, struct page *page)
{
struct fuse_io_priv io = { .async = 0, .file = file };
@@ -691,18 +700,18 @@ static int fuse_readpage(struct file *file, struct page *page)
req->page_descs[0].length = count;
num_read = fuse_send_read(req, &io, pos, count, NULL);
err = req->out.h.error;
- fuse_put_request(fc, req);
if (!err) {
/*
* Short read means EOF. If file size is larger, truncate it
*/
if (num_read < count)
- fuse_read_update_size(inode, pos + num_read, attr_ver);
+ fuse_short_read(req, inode, attr_ver);
SetPageUptodate(page);
}
+ fuse_put_request(fc, req);
fuse_invalidate_attr(inode); /* atime changed */
out:
unlock_page(page);
@@ -725,13 +734,9 @@ static void fuse_readpages_end(struct fuse_conn *fc, struct fuse_req *req)
/*
* Short read means EOF. If file size is larger, truncate it
*/
- if (!req->out.h.error && num_read < count) {
- loff_t pos;
+ if (!req->out.h.error && num_read < count)
+ fuse_short_read(req, inode, req->misc.read.attr_ver);
- pos = page_offset(req->pages[0]) + num_read;
- fuse_read_update_size(inode, pos,
- req->misc.read.attr_ver);
- }
fuse_invalidate_attr(inode); /* atime changed */
}
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 03/11] fuse: Connection bit for enabling writeback
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
2013-10-10 13:10 ` [PATCH 01/11] fuse: Linking file to inode helper Maxim Patlasov
2013-10-10 13:10 ` [PATCH 02/11] fuse: Prepare to handle short reads Maxim Patlasov
@ 2013-10-10 13:10 ` Maxim Patlasov
2013-10-10 13:10 ` [PATCH 04/11] fuse: Trust kernel i_size only - v4 Maxim Patlasov
` (8 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:10 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
Off (0) by default. Will be used in the next patches and will be turned
on at the very end.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
---
fs/fuse/fuse_i.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index a08b355..a490ba3 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -479,6 +479,9 @@ struct fuse_conn {
/** Set if bdi is valid */
unsigned bdi_initialized:1;
+ /** write-back cache policy (default is write-through) */
+ unsigned writeback_cache:1;
+
/*
* The following bitfields are only for optimization purposes
* and hence races in setting them will not cause malfunction
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 04/11] fuse: Trust kernel i_size only - v4
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (2 preceding siblings ...)
2013-10-10 13:10 ` [PATCH 03/11] fuse: Connection bit for enabling writeback Maxim Patlasov
@ 2013-10-10 13:10 ` Maxim Patlasov
[not found] ` <20131010130718.10089.6736.stgit-opBMJL+S1+lZ2WjqXT1pg9yJl3JzOD/t@public.gmane.org>
` (7 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:10 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
Make fuse think that when writeback is on the inode's i_size is always
up-to-date and not update it with the value received from the userspace.
This is done because the page cache code may update i_size without letting
the FS know.
This assumption implies fixing the previously introduced short-read helper --
when a short read occurs the 'hole' is filled with zeroes.
fuse_file_fallocate() is also fixed because now we should keep i_size up to
date, so it must be updated if FUSE_FALLOCATE request succeeded.
Changed in v2:
- improved comment in fuse_short_read()
- fixed fuse_file_fallocate() for KEEP_SIZE mode
Changed in v3:
- fixed fuse_fillattr() not to use local i_size if writeback-cache is off
- added a comment explaining why we cannot trust attr.size from server
Changed in v4:
- do not change fuse_file_fallocate() because what we need is already
implemented in commits a200a2d and 14c1441
Signed-off-by: Maxim V. Patlasov <MPatlasov@parallels.com>
---
fs/fuse/dir.c | 13 +++++++++++--
fs/fuse/file.c | 26 ++++++++++++++++++++++++--
fs/fuse/inode.c | 11 +++++++++--
3 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 62b43b5..f022968 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -854,6 +854,11 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
struct kstat *stat)
{
unsigned int blkbits;
+ struct fuse_conn *fc = get_fuse_conn(inode);
+
+ /* see the comment in fuse_change_attributes() */
+ if (fc->writeback_cache && S_ISREG(inode->i_mode))
+ attr->size = i_size_read(inode);
stat->dev = inode->i_sb->s_dev;
stat->ino = attr->ino;
@@ -1594,6 +1599,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
struct fuse_setattr_in inarg;
struct fuse_attr_out outarg;
bool is_truncate = false;
+ bool is_wb = fc->writeback_cache;
loff_t oldsize;
int err;
@@ -1665,7 +1671,9 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
fuse_change_attributes_common(inode, &outarg.attr,
attr_timeout(&outarg));
oldsize = inode->i_size;
- i_size_write(inode, outarg.attr.size);
+ /* see the comment in fuse_change_attributes() */
+ if (!is_wb || is_truncate || !S_ISREG(inode->i_mode))
+ i_size_write(inode, outarg.attr.size);
if (is_truncate) {
/* NOTE: this may release/reacquire fc->lock */
@@ -1677,7 +1685,8 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
* Only call invalidate_inode_pages2() after removing
* FUSE_NOWRITE, otherwise fuse_launder_page() would deadlock.
*/
- if (S_ISREG(inode->i_mode) && oldsize != outarg.attr.size) {
+ if ((is_truncate || !is_wb) &&
+ S_ISREG(inode->i_mode) && oldsize != outarg.attr.size) {
truncate_pagecache(inode, outarg.attr.size);
invalidate_inode_pages2(inode->i_mapping);
}
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 886f3a2..333a764 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -658,9 +658,31 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
u64 attr_ver)
{
size_t num_read = req->out.args[0].size;
+ struct fuse_conn *fc = get_fuse_conn(inode);
+
+ if (fc->writeback_cache) {
+ /*
+ * A hole in a file. Some data after the hole are in page cache,
+ * but have not reached the client fs yet. So, the hole is not
+ * present there.
+ */
+ int i;
+ int start_idx = num_read >> PAGE_CACHE_SHIFT;
+ size_t off = num_read & (PAGE_CACHE_SIZE - 1);
+
+ for (i = start_idx; i < req->num_pages; i++) {
+ struct page *page = req->pages[i];
+ void *mapaddr = kmap_atomic(page);
- loff_t pos = page_offset(req->pages[0]) + num_read;
- fuse_read_update_size(inode, pos, attr_ver);
+ memset(mapaddr + off, 0, PAGE_CACHE_SIZE - off);
+
+ kunmap_atomic(mapaddr);
+ off = 0;
+ }
+ } else {
+ loff_t pos = page_offset(req->pages[0]) + num_read;
+ fuse_read_update_size(inode, pos, attr_ver);
+ }
}
static int fuse_readpage(struct file *file, struct page *page)
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index a8ce6da..63818ab 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -197,6 +197,7 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
{
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
+ bool is_wb = fc->writeback_cache;
loff_t oldsize;
struct timespec old_mtime;
@@ -211,10 +212,16 @@ void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
fuse_change_attributes_common(inode, attr, attr_valid);
oldsize = inode->i_size;
- i_size_write(inode, attr->size);
+ /*
+ * In case of writeback_cache enabled, the cached writes beyond EOF
+ * extend local i_size without keeping userspace server in sync. So,
+ * attr->size coming from server can be stale. We cannot trust it.
+ */
+ if (!is_wb || !S_ISREG(inode->i_mode))
+ i_size_write(inode, attr->size);
spin_unlock(&fc->lock);
- if (S_ISREG(inode->i_mode)) {
+ if (!is_wb && S_ISREG(inode->i_mode)) {
bool inval = false;
if (oldsize != attr->size) {
^ permalink raw reply related [flat|nested] 23+ messages in thread[parent not found: <20131010130718.10089.6736.stgit-opBMJL+S1+lZ2WjqXT1pg9yJl3JzOD/t@public.gmane.org>]
* [PATCH 05/11] fuse: Trust kernel i_mtime only -v2
[not found] ` <20131010130718.10089.6736.stgit-opBMJL+S1+lZ2WjqXT1pg9yJl3JzOD/t@public.gmane.org>
@ 2013-10-10 13:10 ` Maxim Patlasov
2013-11-12 16:52 ` Miklos Szeredi
0 siblings, 1 reply; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:10 UTC (permalink / raw)
To: miklos-sUDqSbJrdHQHWmgEVkV9KA
Cc: dev-bzQdu9zFT3WakBO8gow8eQ, xemul-bzQdu9zFT3WakBO8gow8eQ,
fuse-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jbottomley-bzQdu9zFT3WakBO8gow8eQ,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
fengguang.wu-ral2JQCrhuEAvxtiuMwx3w, devel-GEFAQzZX7r8dnm+yROfE0A
Let the kernel maintain i_mtime locally:
- clear S_NOCMTIME
- implement i_op->update_time()
- flush mtime on fsync and last close
- update i_mtime explicitly on truncate and fallocate
Fuse inode flag FUSE_I_MTIME_DIRTY serves as indication that local i_mtime
should be flushed to the server eventually.
Changed in v2 (thanks to Brian):
- renamed FUSE_I_MTIME_UPDATED to FUSE_I_MTIME_DIRTY
- simplified fuse_set_mtime_local()
- abandoned optimizations: clearing the flag on some operations (like
direct write) is doable, but may lead to unexpected changes of
user-visible mtime.
Signed-off-by: Maxim Patlasov <MPatlasov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
---
fs/fuse/dir.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++------
fs/fuse/file.c | 30 +++++++++++++--
fs/fuse/fuse_i.h | 6 ++-
fs/fuse/inode.c | 13 +++++-
4 files changed, 138 insertions(+), 20 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index f022968..eda248b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -857,8 +857,11 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
struct fuse_conn *fc = get_fuse_conn(inode);
/* see the comment in fuse_change_attributes() */
- if (fc->writeback_cache && S_ISREG(inode->i_mode))
+ if (fc->writeback_cache && S_ISREG(inode->i_mode)) {
attr->size = i_size_read(inode);
+ attr->mtime = inode->i_mtime.tv_sec;
+ attr->mtimensec = inode->i_mtime.tv_nsec;
+ }
stat->dev = inode->i_sb->s_dev;
stat->ino = attr->ino;
@@ -1582,6 +1585,82 @@ void fuse_release_nowrite(struct inode *inode)
spin_unlock(&fc->lock);
}
+static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
+ struct inode *inode,
+ struct fuse_setattr_in *inarg_p,
+ struct fuse_attr_out *outarg_p)
+{
+ req->in.h.opcode = FUSE_SETATTR;
+ req->in.h.nodeid = get_node_id(inode);
+ req->in.numargs = 1;
+ req->in.args[0].size = sizeof(*inarg_p);
+ req->in.args[0].value = inarg_p;
+ req->out.numargs = 1;
+ if (fc->minor < 9)
+ req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
+ else
+ req->out.args[0].size = sizeof(*outarg_p);
+ req->out.args[0].value = outarg_p;
+}
+
+/*
+ * Flush inode->i_mtime to the server
+ */
+int fuse_flush_mtime(struct file *file, bool nofail)
+{
+ struct inode *inode = file->f_mapping->host;
+ struct fuse_inode *fi = get_fuse_inode(inode);
+ struct fuse_conn *fc = get_fuse_conn(inode);
+ struct fuse_req *req = NULL;
+ struct fuse_setattr_in inarg;
+ struct fuse_attr_out outarg;
+ int err;
+
+ if (nofail) {
+ req = fuse_get_req_nofail_nopages(fc, file);
+ } else {
+ req = fuse_get_req_nopages(fc);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+ }
+
+ memset(&inarg, 0, sizeof(inarg));
+ memset(&outarg, 0, sizeof(outarg));
+
+ inarg.valid |= FATTR_MTIME;
+ inarg.mtime = inode->i_mtime.tv_sec;
+ inarg.mtimensec = inode->i_mtime.tv_nsec;
+
+ fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
+ fuse_request_send(fc, req);
+ err = req->out.h.error;
+ fuse_put_request(fc, req);
+
+ if (!err)
+ clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+
+ return err;
+}
+
+/*
+ * S_NOCMTIME is clear, so we need to update inode->i_mtime manually.
+ */
+static void fuse_set_mtime_local(struct iattr *iattr, struct inode *inode)
+{
+ unsigned ivalid = iattr->ia_valid;
+ struct fuse_inode *fi = get_fuse_inode(inode);
+
+ if ((ivalid & ATTR_MTIME) && (ivalid & ATTR_MTIME_SET)) {
+ /* client fs has just set mtime to iattr->ia_mtime */
+ inode->i_mtime = iattr->ia_mtime;
+ clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+ } else if ((ivalid & ATTR_MTIME) || (ivalid & ATTR_SIZE)) {
+ /* client fs doesn't know that we're updating i_mtime */
+ inode->i_mtime = current_fs_time(inode->i_sb);
+ set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+ }
+}
+
/*
* Set attributes, and at the same time refresh them.
*
@@ -1641,17 +1720,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
inarg.valid |= FATTR_LOCKOWNER;
inarg.lock_owner = fuse_lock_owner_id(fc, current->files);
}
- req->in.h.opcode = FUSE_SETATTR;
- req->in.h.nodeid = get_node_id(inode);
- req->in.numargs = 1;
- req->in.args[0].size = sizeof(inarg);
- req->in.args[0].value = &inarg;
- req->out.numargs = 1;
- if (fc->minor < 9)
- req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
- else
- req->out.args[0].size = sizeof(outarg);
- req->out.args[0].value = &outarg;
+ fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
fuse_request_send(fc, req);
err = req->out.h.error;
fuse_put_request(fc, req);
@@ -1668,6 +1737,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
}
spin_lock(&fc->lock);
+ /* the kernel maintains i_mtime locally */
+ if (fc->writeback_cache && S_ISREG(inode->i_mode))
+ fuse_set_mtime_local(attr, inode);
+
fuse_change_attributes_common(inode, &outarg.attr,
attr_timeout(&outarg));
oldsize = inode->i_size;
@@ -1898,6 +1971,17 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
return err;
}
+static int fuse_update_time(struct inode *inode, struct timespec *now,
+ int flags)
+{
+ if (flags & S_MTIME) {
+ inode->i_mtime = *now;
+ set_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state);
+ BUG_ON(!S_ISREG(inode->i_mode));
+ }
+ return 0;
+}
+
static const struct inode_operations fuse_dir_inode_operations = {
.lookup = fuse_lookup,
.mkdir = fuse_mkdir,
@@ -1937,6 +2021,7 @@ static const struct inode_operations fuse_common_inode_operations = {
.getxattr = fuse_getxattr,
.listxattr = fuse_listxattr,
.removexattr = fuse_removexattr,
+ .update_time = fuse_update_time,
};
static const struct inode_operations fuse_symlink_inode_operations = {
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 333a764..eabe202 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -291,6 +291,9 @@ static int fuse_open(struct inode *inode, struct file *file)
static int fuse_release(struct inode *inode, struct file *file)
{
+ if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state))
+ fuse_flush_mtime(file, true);
+
fuse_release_common(file, FUSE_RELEASE);
/* return value is ignored by VFS */
@@ -458,6 +461,12 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
fuse_sync_writes(inode);
+ if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state)) {
+ int err = fuse_flush_mtime(file, false);
+ if (err)
+ goto out;
+ }
+
req = fuse_get_req_nopages(fc);
if (IS_ERR(req)) {
err = PTR_ERR(req);
@@ -948,16 +957,21 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
return req->misc.write.out.size;
}
-void fuse_write_update_size(struct inode *inode, loff_t pos)
+bool fuse_write_update_size(struct inode *inode, loff_t pos)
{
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
+ bool ret = false;
spin_lock(&fc->lock);
fi->attr_version = ++fc->attr_version;
- if (pos > inode->i_size)
+ if (pos > inode->i_size) {
i_size_write(inode, pos);
+ ret = true;
+ }
spin_unlock(&fc->lock);
+
+ return ret;
}
static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
@@ -2862,8 +2876,16 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
goto out;
/* we could have extended the file */
- if (!(mode & FALLOC_FL_KEEP_SIZE))
- fuse_write_update_size(inode, offset + length);
+ if (!(mode & FALLOC_FL_KEEP_SIZE)) {
+ bool changed = fuse_write_update_size(inode, offset + length);
+
+ if (changed && fc->writeback_cache) {
+ struct fuse_inode *fi = get_fuse_inode(inode);
+
+ inode->i_mtime = current_fs_time(inode->i_sb);
+ set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+ }
+ }
if (mode & FALLOC_FL_PUNCH_HOLE)
truncate_pagecache_range(inode, offset, offset + length - 1);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index a490ba3..3028b8a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -117,6 +117,8 @@ enum {
FUSE_I_ADVISE_RDPLUS,
/** An operation changing file size is in progress */
FUSE_I_SIZE_UNSTABLE,
+ /** i_mtime has been updated locally; a flush to userspace needed */
+ FUSE_I_MTIME_DIRTY,
};
struct fuse_conn;
@@ -870,7 +872,9 @@ long fuse_ioctl_common(struct file *file, unsigned int cmd,
unsigned fuse_file_poll(struct file *file, poll_table *wait);
int fuse_dev_release(struct inode *inode, struct file *file);
-void fuse_write_update_size(struct inode *inode, loff_t pos);
+bool fuse_write_update_size(struct inode *inode, loff_t pos);
+
+int fuse_flush_mtime(struct file *file, bool nofail);
int fuse_do_setattr(struct inode *inode, struct iattr *attr,
struct file *file);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 63818ab..253701f 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -170,8 +170,11 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
inode->i_blocks = attr->blocks;
inode->i_atime.tv_sec = attr->atime;
inode->i_atime.tv_nsec = attr->atimensec;
- inode->i_mtime.tv_sec = attr->mtime;
- inode->i_mtime.tv_nsec = attr->mtimensec;
+ /* mtime from server may be stale due to local buffered write */
+ if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) {
+ inode->i_mtime.tv_sec = attr->mtime;
+ inode->i_mtime.tv_nsec = attr->mtimensec;
+ }
inode->i_ctime.tv_sec = attr->ctime;
inode->i_ctime.tv_nsec = attr->ctimensec;
@@ -250,6 +253,8 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
{
inode->i_mode = attr->mode & S_IFMT;
inode->i_size = attr->size;
+ inode->i_mtime.tv_sec = attr->mtime;
+ inode->i_mtime.tv_nsec = attr->mtimensec;
if (S_ISREG(inode->i_mode)) {
fuse_init_common(inode);
fuse_init_file_inode(inode);
@@ -296,7 +301,9 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
return NULL;
if ((inode->i_state & I_NEW)) {
- inode->i_flags |= S_NOATIME|S_NOCMTIME;
+ inode->i_flags |= S_NOATIME;
+ if (!fc->writeback_cache)
+ inode->i_flags |= S_NOCMTIME;
inode->i_generation = generation;
inode->i_data.backing_dev_info = &fc->bdi;
fuse_init_inode(inode, attr);
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60134071&iu=/4140/ostg.clktrk
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 05/11] fuse: Trust kernel i_mtime only -v2
2013-10-10 13:10 ` [PATCH 05/11] fuse: Trust kernel i_mtime only -v2 Maxim Patlasov
@ 2013-11-12 16:52 ` Miklos Szeredi
2013-12-26 15:41 ` Maxim Patlasov
2013-12-26 15:51 ` [PATCH 05/11] fuse: Trust kernel i_mtime only -v3 Maxim Patlasov
0 siblings, 2 replies; 23+ messages in thread
From: Miklos Szeredi @ 2013-11-12 16:52 UTC (permalink / raw)
To: Maxim Patlasov
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
On Thu, Oct 10, 2013 at 05:10:56PM +0400, Maxim Patlasov wrote:
> Let the kernel maintain i_mtime locally:
> - clear S_NOCMTIME
> - implement i_op->update_time()
> - flush mtime on fsync and last close
> - update i_mtime explicitly on truncate and fallocate
>
> Fuse inode flag FUSE_I_MTIME_DIRTY serves as indication that local i_mtime
> should be flushed to the server eventually.
>
> Changed in v2 (thanks to Brian):
> - renamed FUSE_I_MTIME_UPDATED to FUSE_I_MTIME_DIRTY
> - simplified fuse_set_mtime_local()
> - abandoned optimizations: clearing the flag on some operations (like
> direct write) is doable, but may lead to unexpected changes of
> user-visible mtime.
>
> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
> ---
> fs/fuse/dir.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++------
> fs/fuse/file.c | 30 +++++++++++++--
> fs/fuse/fuse_i.h | 6 ++-
> fs/fuse/inode.c | 13 +++++-
> 4 files changed, 138 insertions(+), 20 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index f022968..eda248b 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -857,8 +857,11 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
> struct fuse_conn *fc = get_fuse_conn(inode);
>
> /* see the comment in fuse_change_attributes() */
> - if (fc->writeback_cache && S_ISREG(inode->i_mode))
> + if (fc->writeback_cache && S_ISREG(inode->i_mode)) {
> attr->size = i_size_read(inode);
> + attr->mtime = inode->i_mtime.tv_sec;
> + attr->mtimensec = inode->i_mtime.tv_nsec;
> + }
>
> stat->dev = inode->i_sb->s_dev;
> stat->ino = attr->ino;
> @@ -1582,6 +1585,82 @@ void fuse_release_nowrite(struct inode *inode)
> spin_unlock(&fc->lock);
> }
>
> +static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
> + struct inode *inode,
> + struct fuse_setattr_in *inarg_p,
> + struct fuse_attr_out *outarg_p)
> +{
> + req->in.h.opcode = FUSE_SETATTR;
> + req->in.h.nodeid = get_node_id(inode);
> + req->in.numargs = 1;
> + req->in.args[0].size = sizeof(*inarg_p);
> + req->in.args[0].value = inarg_p;
> + req->out.numargs = 1;
> + if (fc->minor < 9)
> + req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
> + else
> + req->out.args[0].size = sizeof(*outarg_p);
> + req->out.args[0].value = outarg_p;
> +}
> +
> +/*
> + * Flush inode->i_mtime to the server
> + */
> +int fuse_flush_mtime(struct file *file, bool nofail)
> +{
> + struct inode *inode = file->f_mapping->host;
> + struct fuse_inode *fi = get_fuse_inode(inode);
> + struct fuse_conn *fc = get_fuse_conn(inode);
> + struct fuse_req *req = NULL;
> + struct fuse_setattr_in inarg;
> + struct fuse_attr_out outarg;
> + int err;
> +
> + if (nofail) {
> + req = fuse_get_req_nofail_nopages(fc, file);
> + } else {
> + req = fuse_get_req_nopages(fc);
> + if (IS_ERR(req))
> + return PTR_ERR(req);
> + }
> +
> + memset(&inarg, 0, sizeof(inarg));
> + memset(&outarg, 0, sizeof(outarg));
> +
> + inarg.valid |= FATTR_MTIME;
> + inarg.mtime = inode->i_mtime.tv_sec;
> + inarg.mtimensec = inode->i_mtime.tv_nsec;
> +
> + fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
> + fuse_request_send(fc, req);
> + err = req->out.h.error;
> + fuse_put_request(fc, req);
> +
> + if (!err)
> + clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
Doing the test and the clear separately opens a huge race window when i_mtime
modifications are bound to get lost.
> +
> + return err;
> +}
> +
> +/*
> + * S_NOCMTIME is clear, so we need to update inode->i_mtime manually.
> + */
> +static void fuse_set_mtime_local(struct iattr *iattr, struct inode *inode)
> +{
> + unsigned ivalid = iattr->ia_valid;
> + struct fuse_inode *fi = get_fuse_inode(inode);
> +
> + if ((ivalid & ATTR_MTIME) && (ivalid & ATTR_MTIME_SET)) {
> + /* client fs has just set mtime to iattr->ia_mtime */
> + inode->i_mtime = iattr->ia_mtime;
> + clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
This is protected by i_mutex, so it should be safe.
> + } else if ((ivalid & ATTR_MTIME) || (ivalid & ATTR_SIZE)) {
> + /* client fs doesn't know that we're updating i_mtime */
If so, why not tell the client fs to update mtime?
> + inode->i_mtime = current_fs_time(inode->i_sb);
> + set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> + }
> +}
> +
> /*
> * Set attributes, and at the same time refresh them.
> *
> @@ -1641,17 +1720,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> inarg.valid |= FATTR_LOCKOWNER;
> inarg.lock_owner = fuse_lock_owner_id(fc, current->files);
> }
> - req->in.h.opcode = FUSE_SETATTR;
> - req->in.h.nodeid = get_node_id(inode);
> - req->in.numargs = 1;
> - req->in.args[0].size = sizeof(inarg);
> - req->in.args[0].value = &inarg;
> - req->out.numargs = 1;
> - if (fc->minor < 9)
> - req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
> - else
> - req->out.args[0].size = sizeof(outarg);
> - req->out.args[0].value = &outarg;
> + fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
> fuse_request_send(fc, req);
> err = req->out.h.error;
> fuse_put_request(fc, req);
> @@ -1668,6 +1737,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> }
>
> spin_lock(&fc->lock);
> + /* the kernel maintains i_mtime locally */
> + if (fc->writeback_cache && S_ISREG(inode->i_mode))
> + fuse_set_mtime_local(attr, inode);
> +
> fuse_change_attributes_common(inode, &outarg.attr,
> attr_timeout(&outarg));
> oldsize = inode->i_size;
> @@ -1898,6 +1971,17 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
> return err;
> }
>
> +static int fuse_update_time(struct inode *inode, struct timespec *now,
> + int flags)
> +{
> + if (flags & S_MTIME) {
> + inode->i_mtime = *now;
> + set_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state);
> + BUG_ON(!S_ISREG(inode->i_mode));
> + }
> + return 0;
> +}
> +
> static const struct inode_operations fuse_dir_inode_operations = {
> .lookup = fuse_lookup,
> .mkdir = fuse_mkdir,
> @@ -1937,6 +2021,7 @@ static const struct inode_operations fuse_common_inode_operations = {
> .getxattr = fuse_getxattr,
> .listxattr = fuse_listxattr,
> .removexattr = fuse_removexattr,
> + .update_time = fuse_update_time,
> };
>
> static const struct inode_operations fuse_symlink_inode_operations = {
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 333a764..eabe202 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -291,6 +291,9 @@ static int fuse_open(struct inode *inode, struct file *file)
>
> static int fuse_release(struct inode *inode, struct file *file)
> {
> + if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state))
So, why not make this test_and_clear_bit()?
> + fuse_flush_mtime(file, true);
> +
> fuse_release_common(file, FUSE_RELEASE);
>
> /* return value is ignored by VFS */
> @@ -458,6 +461,12 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>
> fuse_sync_writes(inode);
>
> + if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state)) {
And this too.
> + int err = fuse_flush_mtime(file, false);
> + if (err)
> + goto out;
> + }
> +
> req = fuse_get_req_nopages(fc);
> if (IS_ERR(req)) {
> err = PTR_ERR(req);
> @@ -948,16 +957,21 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
> return req->misc.write.out.size;
> }
>
> -void fuse_write_update_size(struct inode *inode, loff_t pos)
> +bool fuse_write_update_size(struct inode *inode, loff_t pos)
> {
> struct fuse_conn *fc = get_fuse_conn(inode);
> struct fuse_inode *fi = get_fuse_inode(inode);
> + bool ret = false;
>
> spin_lock(&fc->lock);
> fi->attr_version = ++fc->attr_version;
> - if (pos > inode->i_size)
> + if (pos > inode->i_size) {
> i_size_write(inode, pos);
> + ret = true;
> + }
> spin_unlock(&fc->lock);
> +
> + return ret;
> }
>
> static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
> @@ -2862,8 +2876,16 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
> goto out;
>
> /* we could have extended the file */
> - if (!(mode & FALLOC_FL_KEEP_SIZE))
> - fuse_write_update_size(inode, offset + length);
> + if (!(mode & FALLOC_FL_KEEP_SIZE)) {
> + bool changed = fuse_write_update_size(inode, offset + length);
> +
> + if (changed && fc->writeback_cache) {
> + struct fuse_inode *fi = get_fuse_inode(inode);
> +
> + inode->i_mtime = current_fs_time(inode->i_sb);
> + set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> + }
> + }
>
> if (mode & FALLOC_FL_PUNCH_HOLE)
> truncate_pagecache_range(inode, offset, offset + length - 1);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index a490ba3..3028b8a 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -117,6 +117,8 @@ enum {
> FUSE_I_ADVISE_RDPLUS,
> /** An operation changing file size is in progress */
> FUSE_I_SIZE_UNSTABLE,
> + /** i_mtime has been updated locally; a flush to userspace needed */
> + FUSE_I_MTIME_DIRTY,
> };
>
> struct fuse_conn;
> @@ -870,7 +872,9 @@ long fuse_ioctl_common(struct file *file, unsigned int cmd,
> unsigned fuse_file_poll(struct file *file, poll_table *wait);
> int fuse_dev_release(struct inode *inode, struct file *file);
>
> -void fuse_write_update_size(struct inode *inode, loff_t pos);
> +bool fuse_write_update_size(struct inode *inode, loff_t pos);
> +
> +int fuse_flush_mtime(struct file *file, bool nofail);
>
> int fuse_do_setattr(struct inode *inode, struct iattr *attr,
> struct file *file);
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 63818ab..253701f 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -170,8 +170,11 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
> inode->i_blocks = attr->blocks;
> inode->i_atime.tv_sec = attr->atime;
> inode->i_atime.tv_nsec = attr->atimensec;
> - inode->i_mtime.tv_sec = attr->mtime;
> - inode->i_mtime.tv_nsec = attr->mtimensec;
> + /* mtime from server may be stale due to local buffered write */
> + if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) {
> + inode->i_mtime.tv_sec = attr->mtime;
> + inode->i_mtime.tv_nsec = attr->mtimensec;
> + }
> inode->i_ctime.tv_sec = attr->ctime;
> inode->i_ctime.tv_nsec = attr->ctimensec;
>
> @@ -250,6 +253,8 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
> {
> inode->i_mode = attr->mode & S_IFMT;
> inode->i_size = attr->size;
> + inode->i_mtime.tv_sec = attr->mtime;
> + inode->i_mtime.tv_nsec = attr->mtimensec;
> if (S_ISREG(inode->i_mode)) {
> fuse_init_common(inode);
> fuse_init_file_inode(inode);
> @@ -296,7 +301,9 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
> return NULL;
>
> if ((inode->i_state & I_NEW)) {
> - inode->i_flags |= S_NOATIME|S_NOCMTIME;
> + inode->i_flags |= S_NOATIME;
> + if (!fc->writeback_cache)
> + inode->i_flags |= S_NOCMTIME;
> inode->i_generation = generation;
> inode->i_data.backing_dev_info = &fc->bdi;
> fuse_init_inode(inode, attr);
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 05/11] fuse: Trust kernel i_mtime only -v2
2013-11-12 16:52 ` Miklos Szeredi
@ 2013-12-26 15:41 ` Maxim Patlasov
2014-01-06 16:22 ` Miklos Szeredi
2013-12-26 15:51 ` [PATCH 05/11] fuse: Trust kernel i_mtime only -v3 Maxim Patlasov
1 sibling, 1 reply; 23+ messages in thread
From: Maxim Patlasov @ 2013-12-26 15:41 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
Hi Miklos,
Sorry for delay, see please inline comments below.
On 11/12/2013 08:52 PM, Miklos Szeredi wrote:
> On Thu, Oct 10, 2013 at 05:10:56PM +0400, Maxim Patlasov wrote:
>> Let the kernel maintain i_mtime locally:
>> - clear S_NOCMTIME
>> - implement i_op->update_time()
>> - flush mtime on fsync and last close
>> - update i_mtime explicitly on truncate and fallocate
>>
>> Fuse inode flag FUSE_I_MTIME_DIRTY serves as indication that local i_mtime
>> should be flushed to the server eventually.
>>
>> Changed in v2 (thanks to Brian):
>> - renamed FUSE_I_MTIME_UPDATED to FUSE_I_MTIME_DIRTY
>> - simplified fuse_set_mtime_local()
>> - abandoned optimizations: clearing the flag on some operations (like
>> direct write) is doable, but may lead to unexpected changes of
>> user-visible mtime.
>>
>> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
>> ---
>> fs/fuse/dir.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++------
>> fs/fuse/file.c | 30 +++++++++++++--
>> fs/fuse/fuse_i.h | 6 ++-
>> fs/fuse/inode.c | 13 +++++-
>> 4 files changed, 138 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>> index f022968..eda248b 100644
>> --- a/fs/fuse/dir.c
>> +++ b/fs/fuse/dir.c
>> @@ -857,8 +857,11 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
>> struct fuse_conn *fc = get_fuse_conn(inode);
>>
>> /* see the comment in fuse_change_attributes() */
>> - if (fc->writeback_cache && S_ISREG(inode->i_mode))
>> + if (fc->writeback_cache && S_ISREG(inode->i_mode)) {
>> attr->size = i_size_read(inode);
>> + attr->mtime = inode->i_mtime.tv_sec;
>> + attr->mtimensec = inode->i_mtime.tv_nsec;
>> + }
>>
>> stat->dev = inode->i_sb->s_dev;
>> stat->ino = attr->ino;
>> @@ -1582,6 +1585,82 @@ void fuse_release_nowrite(struct inode *inode)
>> spin_unlock(&fc->lock);
>> }
>>
>> +static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
>> + struct inode *inode,
>> + struct fuse_setattr_in *inarg_p,
>> + struct fuse_attr_out *outarg_p)
>> +{
>> + req->in.h.opcode = FUSE_SETATTR;
>> + req->in.h.nodeid = get_node_id(inode);
>> + req->in.numargs = 1;
>> + req->in.args[0].size = sizeof(*inarg_p);
>> + req->in.args[0].value = inarg_p;
>> + req->out.numargs = 1;
>> + if (fc->minor < 9)
>> + req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
>> + else
>> + req->out.args[0].size = sizeof(*outarg_p);
>> + req->out.args[0].value = outarg_p;
>> +}
>> +
>> +/*
>> + * Flush inode->i_mtime to the server
>> + */
>> +int fuse_flush_mtime(struct file *file, bool nofail)
>> +{
>> + struct inode *inode = file->f_mapping->host;
>> + struct fuse_inode *fi = get_fuse_inode(inode);
>> + struct fuse_conn *fc = get_fuse_conn(inode);
>> + struct fuse_req *req = NULL;
>> + struct fuse_setattr_in inarg;
>> + struct fuse_attr_out outarg;
>> + int err;
>> +
>> + if (nofail) {
>> + req = fuse_get_req_nofail_nopages(fc, file);
>> + } else {
>> + req = fuse_get_req_nopages(fc);
>> + if (IS_ERR(req))
>> + return PTR_ERR(req);
>> + }
>> +
>> + memset(&inarg, 0, sizeof(inarg));
>> + memset(&outarg, 0, sizeof(outarg));
>> +
>> + inarg.valid |= FATTR_MTIME;
>> + inarg.mtime = inode->i_mtime.tv_sec;
>> + inarg.mtimensec = inode->i_mtime.tv_nsec;
>> +
>> + fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>> + fuse_request_send(fc, req);
>> + err = req->out.h.error;
>> + fuse_put_request(fc, req);
>> +
>> + if (!err)
>> + clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> Doing the test and the clear separately opens a huge race window when i_mtime
> modifications are bound to get lost.
No. Because the whole operation is protected by i_mutex (see
fuse_fsync_common()).
>
>> +
>> + return err;
>> +}
>> +
>> +/*
>> + * S_NOCMTIME is clear, so we need to update inode->i_mtime manually.
>> + */
>> +static void fuse_set_mtime_local(struct iattr *iattr, struct inode *inode)
>> +{
>> + unsigned ivalid = iattr->ia_valid;
>> + struct fuse_inode *fi = get_fuse_inode(inode);
>> +
>> + if ((ivalid & ATTR_MTIME) && (ivalid & ATTR_MTIME_SET)) {
>> + /* client fs has just set mtime to iattr->ia_mtime */
>> + inode->i_mtime = iattr->ia_mtime;
>> + clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> This is protected by i_mutex, so it should be safe.
Yes.
>
>> + } else if ((ivalid & ATTR_MTIME) || (ivalid & ATTR_SIZE)) {
>> + /* client fs doesn't know that we're updating i_mtime */
> If so, why not tell the client fs to update mtime?
Looks reasonable. I'll resend updated patch soon.
>
>> + inode->i_mtime = current_fs_time(inode->i_sb);
>> + set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
>> + }
>> +}
>> +
>> /*
>> * Set attributes, and at the same time refresh them.
>> *
>> @@ -1641,17 +1720,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>> inarg.valid |= FATTR_LOCKOWNER;
>> inarg.lock_owner = fuse_lock_owner_id(fc, current->files);
>> }
>> - req->in.h.opcode = FUSE_SETATTR;
>> - req->in.h.nodeid = get_node_id(inode);
>> - req->in.numargs = 1;
>> - req->in.args[0].size = sizeof(inarg);
>> - req->in.args[0].value = &inarg;
>> - req->out.numargs = 1;
>> - if (fc->minor < 9)
>> - req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
>> - else
>> - req->out.args[0].size = sizeof(outarg);
>> - req->out.args[0].value = &outarg;
>> + fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
>> fuse_request_send(fc, req);
>> err = req->out.h.error;
>> fuse_put_request(fc, req);
>> @@ -1668,6 +1737,10 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>> }
>>
>> spin_lock(&fc->lock);
>> + /* the kernel maintains i_mtime locally */
>> + if (fc->writeback_cache && S_ISREG(inode->i_mode))
>> + fuse_set_mtime_local(attr, inode);
>> +
>> fuse_change_attributes_common(inode, &outarg.attr,
>> attr_timeout(&outarg));
>> oldsize = inode->i_size;
>> @@ -1898,6 +1971,17 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
>> return err;
>> }
>>
>> +static int fuse_update_time(struct inode *inode, struct timespec *now,
>> + int flags)
>> +{
>> + if (flags & S_MTIME) {
>> + inode->i_mtime = *now;
>> + set_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state);
>> + BUG_ON(!S_ISREG(inode->i_mode));
>> + }
>> + return 0;
>> +}
>> +
>> static const struct inode_operations fuse_dir_inode_operations = {
>> .lookup = fuse_lookup,
>> .mkdir = fuse_mkdir,
>> @@ -1937,6 +2021,7 @@ static const struct inode_operations fuse_common_inode_operations = {
>> .getxattr = fuse_getxattr,
>> .listxattr = fuse_listxattr,
>> .removexattr = fuse_removexattr,
>> + .update_time = fuse_update_time,
>> };
>>
>> static const struct inode_operations fuse_symlink_inode_operations = {
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 333a764..eabe202 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -291,6 +291,9 @@ static int fuse_open(struct inode *inode, struct file *file)
>>
>> static int fuse_release(struct inode *inode, struct file *file)
>> {
>> + if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state))
> So, why not make this test_and_clear_bit()?
To be uniform with fuse_fsync_common(). See please the next comment.
>
>
>> + fuse_flush_mtime(file, true);
>> +
>> fuse_release_common(file, FUSE_RELEASE);
>>
>> /* return value is ignored by VFS */
>> @@ -458,6 +461,12 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
>>
>> fuse_sync_writes(inode);
>>
>> + if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state)) {
> And this too.
If mtime flush fails, the bit is kept. Another way would be to use
test_and_clear_bit() as you suggested, but set the bit back in case of
failure. Technically they are equivalent (due to i_mutex protection),
but current implementation looks more natural.
Thanks,
Maxim
>
>> + int err = fuse_flush_mtime(file, false);
>> + if (err)
>> + goto out;
>> + }
>> +
>> req = fuse_get_req_nopages(fc);
>> if (IS_ERR(req)) {
>> err = PTR_ERR(req);
>> @@ -948,16 +957,21 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
>> return req->misc.write.out.size;
>> }
>>
>> -void fuse_write_update_size(struct inode *inode, loff_t pos)
>> +bool fuse_write_update_size(struct inode *inode, loff_t pos)
>> {
>> struct fuse_conn *fc = get_fuse_conn(inode);
>> struct fuse_inode *fi = get_fuse_inode(inode);
>> + bool ret = false;
>>
>> spin_lock(&fc->lock);
>> fi->attr_version = ++fc->attr_version;
>> - if (pos > inode->i_size)
>> + if (pos > inode->i_size) {
>> i_size_write(inode, pos);
>> + ret = true;
>> + }
>> spin_unlock(&fc->lock);
>> +
>> + return ret;
>> }
>>
>> static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
>> @@ -2862,8 +2876,16 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
>> goto out;
>>
>> /* we could have extended the file */
>> - if (!(mode & FALLOC_FL_KEEP_SIZE))
>> - fuse_write_update_size(inode, offset + length);
>> + if (!(mode & FALLOC_FL_KEEP_SIZE)) {
>> + bool changed = fuse_write_update_size(inode, offset + length);
>> +
>> + if (changed && fc->writeback_cache) {
>> + struct fuse_inode *fi = get_fuse_inode(inode);
>> +
>> + inode->i_mtime = current_fs_time(inode->i_sb);
>> + set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
>> + }
>> + }
>>
>> if (mode & FALLOC_FL_PUNCH_HOLE)
>> truncate_pagecache_range(inode, offset, offset + length - 1);
>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>> index a490ba3..3028b8a 100644
>> --- a/fs/fuse/fuse_i.h
>> +++ b/fs/fuse/fuse_i.h
>> @@ -117,6 +117,8 @@ enum {
>> FUSE_I_ADVISE_RDPLUS,
>> /** An operation changing file size is in progress */
>> FUSE_I_SIZE_UNSTABLE,
>> + /** i_mtime has been updated locally; a flush to userspace needed */
>> + FUSE_I_MTIME_DIRTY,
>> };
>>
>> struct fuse_conn;
>> @@ -870,7 +872,9 @@ long fuse_ioctl_common(struct file *file, unsigned int cmd,
>> unsigned fuse_file_poll(struct file *file, poll_table *wait);
>> int fuse_dev_release(struct inode *inode, struct file *file);
>>
>> -void fuse_write_update_size(struct inode *inode, loff_t pos);
>> +bool fuse_write_update_size(struct inode *inode, loff_t pos);
>> +
>> +int fuse_flush_mtime(struct file *file, bool nofail);
>>
>> int fuse_do_setattr(struct inode *inode, struct iattr *attr,
>> struct file *file);
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 63818ab..253701f 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -170,8 +170,11 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
>> inode->i_blocks = attr->blocks;
>> inode->i_atime.tv_sec = attr->atime;
>> inode->i_atime.tv_nsec = attr->atimensec;
>> - inode->i_mtime.tv_sec = attr->mtime;
>> - inode->i_mtime.tv_nsec = attr->mtimensec;
>> + /* mtime from server may be stale due to local buffered write */
>> + if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) {
>> + inode->i_mtime.tv_sec = attr->mtime;
>> + inode->i_mtime.tv_nsec = attr->mtimensec;
>> + }
>> inode->i_ctime.tv_sec = attr->ctime;
>> inode->i_ctime.tv_nsec = attr->ctimensec;
>>
>> @@ -250,6 +253,8 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
>> {
>> inode->i_mode = attr->mode & S_IFMT;
>> inode->i_size = attr->size;
>> + inode->i_mtime.tv_sec = attr->mtime;
>> + inode->i_mtime.tv_nsec = attr->mtimensec;
>> if (S_ISREG(inode->i_mode)) {
>> fuse_init_common(inode);
>> fuse_init_file_inode(inode);
>> @@ -296,7 +301,9 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
>> return NULL;
>>
>> if ((inode->i_state & I_NEW)) {
>> - inode->i_flags |= S_NOATIME|S_NOCMTIME;
>> + inode->i_flags |= S_NOATIME;
>> + if (!fc->writeback_cache)
>> + inode->i_flags |= S_NOCMTIME;
>> inode->i_generation = generation;
>> inode->i_data.backing_dev_info = &fc->bdi;
>> fuse_init_inode(inode, attr);
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 05/11] fuse: Trust kernel i_mtime only -v2
2013-12-26 15:41 ` Maxim Patlasov
@ 2014-01-06 16:22 ` Miklos Szeredi
2014-01-20 11:33 ` Maxim Patlasov
0 siblings, 1 reply; 23+ messages in thread
From: Miklos Szeredi @ 2014-01-06 16:22 UTC (permalink / raw)
To: Maxim Patlasov
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
On Thu, Dec 26, 2013 at 07:41:41PM +0400, Maxim Patlasov wrote:
> >>+
> >>+ if (!err)
> >>+ clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
> >Doing the test and the clear separately opens a huge race window when i_mtime
> >modifications are bound to get lost.
>
> No. Because the whole operation is protected by i_mutex (see
> fuse_fsync_common()).
fuse_release_common() doesn't have i_mutex. It's probably safe to acquire it,
but is that really needed?
Thanks,
Miklos
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 05/11] fuse: Trust kernel i_mtime only -v2
2014-01-06 16:22 ` Miklos Szeredi
@ 2014-01-20 11:33 ` Maxim Patlasov
0 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2014-01-20 11:33 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
On 01/06/2014 08:22 PM, Miklos Szeredi wrote:
> On Thu, Dec 26, 2013 at 07:41:41PM +0400, Maxim Patlasov wrote:
>>>> +
>>>> + if (!err)
>>>> + clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
>>> Doing the test and the clear separately opens a huge race window when i_mtime
>>> modifications are bound to get lost.
>> No. Because the whole operation is protected by i_mutex (see
>> fuse_fsync_common()).
> fuse_release_common() doesn't have i_mutex. It's probably safe to acquire it,
> but is that really needed?
No, that's not needed, I think. Because by the time of calling
fuse_release(), file->f_count is already zero and no userspace activity
is possible on the file.
Are you OK about -v3 version of the patch (sent 12/26/2013)?
Thanks,
Maxim
>
> Thanks,
> Miklos
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 05/11] fuse: Trust kernel i_mtime only -v3
2013-11-12 16:52 ` Miklos Szeredi
2013-12-26 15:41 ` Maxim Patlasov
@ 2013-12-26 15:51 ` Maxim Patlasov
1 sibling, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-12-26 15:51 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, linux-kernel, jbottomley, linux-fsdevel,
akpm, fengguang.wu, devel
Let the kernel maintain i_mtime locally:
- clear S_NOCMTIME
- implement i_op->update_time()
- flush mtime on fsync and last close
- update i_mtime explicitly on truncate and fallocate
Fuse inode flag FUSE_I_MTIME_DIRTY serves as indication that local i_mtime
should be flushed to the server eventually.
Changed in v2 (thanks to Brian):
- renamed FUSE_I_MTIME_UPDATED to FUSE_I_MTIME_DIRTY
- simplified fuse_set_mtime_local()
- abandoned optimizations: clearing the flag on some operations (like
direct write) is doable, but may lead to unexpected changes of
user-visible mtime.
Changed in v3 (thanks to Miklos):
- let client fs know about local mtime change for ATTR_SIZE and
!ATTR_MTIME_SET cases.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/dir.c | 113 +++++++++++++++++++++++++++++++++++++++++++++---------
fs/fuse/file.c | 30 ++++++++++++--
fs/fuse/fuse_i.h | 6 ++-
fs/fuse/inode.c | 13 +++++-
4 files changed, 136 insertions(+), 26 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index f022968..d1a3167 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -857,8 +857,11 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
struct fuse_conn *fc = get_fuse_conn(inode);
/* see the comment in fuse_change_attributes() */
- if (fc->writeback_cache && S_ISREG(inode->i_mode))
+ if (fc->writeback_cache && S_ISREG(inode->i_mode)) {
attr->size = i_size_read(inode);
+ attr->mtime = inode->i_mtime.tv_sec;
+ attr->mtimensec = inode->i_mtime.tv_nsec;
+ }
stat->dev = inode->i_sb->s_dev;
stat->ino = attr->ino;
@@ -1510,7 +1513,8 @@ static bool update_mtime(unsigned ivalid)
return true;
}
-static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg)
+static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg,
+ bool trust_local_mtime, struct timespec *mtime)
{
unsigned ivalid = iattr->ia_valid;
@@ -1529,11 +1533,17 @@ static void iattr_to_fattr(struct iattr *iattr, struct fuse_setattr_in *arg)
if (!(ivalid & ATTR_ATIME_SET))
arg->valid |= FATTR_ATIME_NOW;
}
- if ((ivalid & ATTR_MTIME) && update_mtime(ivalid)) {
+
+ if (!trust_local_mtime || (ivalid & ATTR_MTIME_SET) ||
+ !(ivalid & ATTR_SIZE))
+ *mtime = iattr->ia_mtime;
+
+ if ((ivalid & ATTR_MTIME) && (update_mtime(ivalid) ||
+ trust_local_mtime)) {
arg->valid |= FATTR_MTIME;
- arg->mtime = iattr->ia_mtime.tv_sec;
- arg->mtimensec = iattr->ia_mtime.tv_nsec;
- if (!(ivalid & ATTR_MTIME_SET))
+ arg->mtime = mtime->tv_sec;
+ arg->mtimensec = mtime->tv_nsec;
+ if (!(ivalid & ATTR_MTIME_SET) && !trust_local_mtime)
arg->valid |= FATTR_MTIME_NOW;
}
}
@@ -1582,6 +1592,63 @@ void fuse_release_nowrite(struct inode *inode)
spin_unlock(&fc->lock);
}
+static void fuse_setattr_fill(struct fuse_conn *fc, struct fuse_req *req,
+ struct inode *inode,
+ struct fuse_setattr_in *inarg_p,
+ struct fuse_attr_out *outarg_p)
+{
+ req->in.h.opcode = FUSE_SETATTR;
+ req->in.h.nodeid = get_node_id(inode);
+ req->in.numargs = 1;
+ req->in.args[0].size = sizeof(*inarg_p);
+ req->in.args[0].value = inarg_p;
+ req->out.numargs = 1;
+ if (fc->minor < 9)
+ req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
+ else
+ req->out.args[0].size = sizeof(*outarg_p);
+ req->out.args[0].value = outarg_p;
+}
+
+/*
+ * Flush inode->i_mtime to the server
+ */
+int fuse_flush_mtime(struct file *file, bool nofail)
+{
+ struct inode *inode = file->f_mapping->host;
+ struct fuse_inode *fi = get_fuse_inode(inode);
+ struct fuse_conn *fc = get_fuse_conn(inode);
+ struct fuse_req *req = NULL;
+ struct fuse_setattr_in inarg;
+ struct fuse_attr_out outarg;
+ int err;
+
+ if (nofail) {
+ req = fuse_get_req_nofail_nopages(fc, file);
+ } else {
+ req = fuse_get_req_nopages(fc);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+ }
+
+ memset(&inarg, 0, sizeof(inarg));
+ memset(&outarg, 0, sizeof(outarg));
+
+ inarg.valid |= FATTR_MTIME;
+ inarg.mtime = inode->i_mtime.tv_sec;
+ inarg.mtimensec = inode->i_mtime.tv_nsec;
+
+ fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
+ fuse_request_send(fc, req);
+ err = req->out.h.error;
+ fuse_put_request(fc, req);
+
+ if (!err)
+ clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+
+ return err;
+}
+
/*
* Set attributes, and at the same time refresh them.
*
@@ -1602,6 +1669,8 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
bool is_wb = fc->writeback_cache;
loff_t oldsize;
int err;
+ bool trust_local_mtime = is_wb && S_ISREG(inode->i_mode);
+ struct timespec new_mtime = current_fs_time(inode->i_sb);
if (!(fc->flags & FUSE_DEFAULT_PERMISSIONS))
attr->ia_valid |= ATTR_FORCE;
@@ -1630,7 +1699,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
memset(&inarg, 0, sizeof(inarg));
memset(&outarg, 0, sizeof(outarg));
- iattr_to_fattr(attr, &inarg);
+ iattr_to_fattr(attr, &inarg, trust_local_mtime, &new_mtime);
if (file) {
struct fuse_file *ff = file->private_data;
inarg.valid |= FATTR_FH;
@@ -1641,17 +1710,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
inarg.valid |= FATTR_LOCKOWNER;
inarg.lock_owner = fuse_lock_owner_id(fc, current->files);
}
- req->in.h.opcode = FUSE_SETATTR;
- req->in.h.nodeid = get_node_id(inode);
- req->in.numargs = 1;
- req->in.args[0].size = sizeof(inarg);
- req->in.args[0].value = &inarg;
- req->out.numargs = 1;
- if (fc->minor < 9)
- req->out.args[0].size = FUSE_COMPAT_ATTR_OUT_SIZE;
- else
- req->out.args[0].size = sizeof(outarg);
- req->out.args[0].value = &outarg;
+ fuse_setattr_fill(fc, req, inode, &inarg, &outarg);
fuse_request_send(fc, req);
err = req->out.h.error;
fuse_put_request(fc, req);
@@ -1668,6 +1727,12 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr,
}
spin_lock(&fc->lock);
+ /* the kernel maintains i_mtime locally */
+ if (trust_local_mtime && (attr->ia_valid & ATTR_MTIME)) {
+ inode->i_mtime = new_mtime;
+ clear_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+ }
+
fuse_change_attributes_common(inode, &outarg.attr,
attr_timeout(&outarg));
oldsize = inode->i_size;
@@ -1898,6 +1963,17 @@ static int fuse_removexattr(struct dentry *entry, const char *name)
return err;
}
+static int fuse_update_time(struct inode *inode, struct timespec *now,
+ int flags)
+{
+ if (flags & S_MTIME) {
+ inode->i_mtime = *now;
+ set_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state);
+ BUG_ON(!S_ISREG(inode->i_mode));
+ }
+ return 0;
+}
+
static const struct inode_operations fuse_dir_inode_operations = {
.lookup = fuse_lookup,
.mkdir = fuse_mkdir,
@@ -1937,6 +2013,7 @@ static const struct inode_operations fuse_common_inode_operations = {
.getxattr = fuse_getxattr,
.listxattr = fuse_listxattr,
.removexattr = fuse_removexattr,
+ .update_time = fuse_update_time,
};
static const struct inode_operations fuse_symlink_inode_operations = {
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 333a764..eabe202 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -291,6 +291,9 @@ static int fuse_open(struct inode *inode, struct file *file)
static int fuse_release(struct inode *inode, struct file *file)
{
+ if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state))
+ fuse_flush_mtime(file, true);
+
fuse_release_common(file, FUSE_RELEASE);
/* return value is ignored by VFS */
@@ -458,6 +461,12 @@ int fuse_fsync_common(struct file *file, loff_t start, loff_t end,
fuse_sync_writes(inode);
+ if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state)) {
+ int err = fuse_flush_mtime(file, false);
+ if (err)
+ goto out;
+ }
+
req = fuse_get_req_nopages(fc);
if (IS_ERR(req)) {
err = PTR_ERR(req);
@@ -948,16 +957,21 @@ static size_t fuse_send_write(struct fuse_req *req, struct fuse_io_priv *io,
return req->misc.write.out.size;
}
-void fuse_write_update_size(struct inode *inode, loff_t pos)
+bool fuse_write_update_size(struct inode *inode, loff_t pos)
{
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
+ bool ret = false;
spin_lock(&fc->lock);
fi->attr_version = ++fc->attr_version;
- if (pos > inode->i_size)
+ if (pos > inode->i_size) {
i_size_write(inode, pos);
+ ret = true;
+ }
spin_unlock(&fc->lock);
+
+ return ret;
}
static size_t fuse_send_write_pages(struct fuse_req *req, struct file *file,
@@ -2862,8 +2876,16 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
goto out;
/* we could have extended the file */
- if (!(mode & FALLOC_FL_KEEP_SIZE))
- fuse_write_update_size(inode, offset + length);
+ if (!(mode & FALLOC_FL_KEEP_SIZE)) {
+ bool changed = fuse_write_update_size(inode, offset + length);
+
+ if (changed && fc->writeback_cache) {
+ struct fuse_inode *fi = get_fuse_inode(inode);
+
+ inode->i_mtime = current_fs_time(inode->i_sb);
+ set_bit(FUSE_I_MTIME_DIRTY, &fi->state);
+ }
+ }
if (mode & FALLOC_FL_PUNCH_HOLE)
truncate_pagecache_range(inode, offset, offset + length - 1);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index a490ba3..3028b8a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -117,6 +117,8 @@ enum {
FUSE_I_ADVISE_RDPLUS,
/** An operation changing file size is in progress */
FUSE_I_SIZE_UNSTABLE,
+ /** i_mtime has been updated locally; a flush to userspace needed */
+ FUSE_I_MTIME_DIRTY,
};
struct fuse_conn;
@@ -870,7 +872,9 @@ long fuse_ioctl_common(struct file *file, unsigned int cmd,
unsigned fuse_file_poll(struct file *file, poll_table *wait);
int fuse_dev_release(struct inode *inode, struct file *file);
-void fuse_write_update_size(struct inode *inode, loff_t pos);
+bool fuse_write_update_size(struct inode *inode, loff_t pos);
+
+int fuse_flush_mtime(struct file *file, bool nofail);
int fuse_do_setattr(struct inode *inode, struct iattr *attr,
struct file *file);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 63818ab..253701f 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -170,8 +170,11 @@ void fuse_change_attributes_common(struct inode *inode, struct fuse_attr *attr,
inode->i_blocks = attr->blocks;
inode->i_atime.tv_sec = attr->atime;
inode->i_atime.tv_nsec = attr->atimensec;
- inode->i_mtime.tv_sec = attr->mtime;
- inode->i_mtime.tv_nsec = attr->mtimensec;
+ /* mtime from server may be stale due to local buffered write */
+ if (!fc->writeback_cache || !S_ISREG(inode->i_mode)) {
+ inode->i_mtime.tv_sec = attr->mtime;
+ inode->i_mtime.tv_nsec = attr->mtimensec;
+ }
inode->i_ctime.tv_sec = attr->ctime;
inode->i_ctime.tv_nsec = attr->ctimensec;
@@ -250,6 +253,8 @@ static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
{
inode->i_mode = attr->mode & S_IFMT;
inode->i_size = attr->size;
+ inode->i_mtime.tv_sec = attr->mtime;
+ inode->i_mtime.tv_nsec = attr->mtimensec;
if (S_ISREG(inode->i_mode)) {
fuse_init_common(inode);
fuse_init_file_inode(inode);
@@ -296,7 +301,9 @@ struct inode *fuse_iget(struct super_block *sb, u64 nodeid,
return NULL;
if ((inode->i_state & I_NEW)) {
- inode->i_flags |= S_NOATIME|S_NOCMTIME;
+ inode->i_flags |= S_NOATIME;
+ if (!fc->writeback_cache)
+ inode->i_flags |= S_NOCMTIME;
inode->i_generation = generation;
inode->i_data.backing_dev_info = &fc->bdi;
fuse_init_inode(inode, attr);
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 06/11] fuse: Flush files on wb close -v2
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (4 preceding siblings ...)
[not found] ` <20131010130718.10089.6736.stgit-opBMJL+S1+lZ2WjqXT1pg9yJl3JzOD/t@public.gmane.org>
@ 2013-10-10 13:11 ` Maxim Patlasov
[not found] ` <20131010131102.10089.51081.stgit-opBMJL+S1+lZ2WjqXT1pg9yJl3JzOD/t@public.gmane.org>
2013-10-10 13:11 ` [PATCH 07/11] fuse: restructure fuse_readpage() Maxim Patlasov
` (5 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:11 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
Changed in v2:
- updated patch to be applied smoothly on top of
"Trust kernel i_mtime only -v2".
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/file.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index eabe202..b4d4189 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -291,6 +291,12 @@ static int fuse_open(struct inode *inode, struct file *file)
static int fuse_release(struct inode *inode, struct file *file)
{
+ struct fuse_conn *fc = get_fuse_conn(inode);
+
+ /* see fuse_vma_close() for !writeback_cache case */
+ if (fc->writeback_cache)
+ filemap_write_and_wait(file->f_mapping);
+
if (test_bit(FUSE_I_MTIME_DIRTY, &get_fuse_inode(inode)->state))
fuse_flush_mtime(file, true);
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 07/11] fuse: restructure fuse_readpage()
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (5 preceding siblings ...)
2013-10-10 13:11 ` [PATCH 06/11] fuse: Flush files on wb close -v2 Maxim Patlasov
@ 2013-10-10 13:11 ` Maxim Patlasov
2013-11-12 17:17 ` Miklos Szeredi
2013-10-10 13:11 ` [PATCH 08/11] fuse: Implement write_begin/write_end callbacks -v2 Maxim Patlasov
` (4 subsequent siblings)
11 siblings, 1 reply; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:11 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
Move the code filling and sending read request to a separate function. Future
patches will use it for .write_begin -- partial modification of a page
requires reading the page from the storage very similarly to what fuse_readpage
does.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/file.c | 55 +++++++++++++++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index b4d4189..77eb849 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -700,21 +700,14 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
}
}
-static int fuse_readpage(struct file *file, struct page *page)
+static int __fuse_readpage(struct file *file, struct page *page, size_t count,
+ int *err, struct fuse_req **req_pp, u64 *attr_ver_p)
{
struct fuse_io_priv io = { .async = 0, .file = file };
struct inode *inode = page->mapping->host;
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_req *req;
size_t num_read;
- loff_t pos = page_offset(page);
- size_t count = PAGE_CACHE_SIZE;
- u64 attr_ver;
- int err;
-
- err = -EIO;
- if (is_bad_inode(inode))
- goto out;
/*
* Page writeback can extend beyond the lifetime of the
@@ -724,20 +717,45 @@ static int fuse_readpage(struct file *file, struct page *page)
fuse_wait_on_page_writeback(inode, page->index);
req = fuse_get_req(fc, 1);
- err = PTR_ERR(req);
+ *err = PTR_ERR(req);
if (IS_ERR(req))
- goto out;
+ return 0;
- attr_ver = fuse_get_attr_version(fc);
+ if (attr_ver_p)
+ *attr_ver_p = fuse_get_attr_version(fc);
req->out.page_zeroing = 1;
req->out.argpages = 1;
req->num_pages = 1;
req->pages[0] = page;
req->page_descs[0].length = count;
- num_read = fuse_send_read(req, &io, pos, count, NULL);
- err = req->out.h.error;
+ num_read = fuse_send_read(req, &io, page_offset(page), count, NULL);
+ *err = req->out.h.error;
+
+ if (*err)
+ fuse_put_request(fc, req);
+ else
+ *req_pp = req;
+
+ return num_read;
+}
+
+static int fuse_readpage(struct file *file, struct page *page)
+{
+ struct inode *inode = page->mapping->host;
+ struct fuse_conn *fc = get_fuse_conn(inode);
+ struct fuse_req *req = NULL;
+ size_t num_read;
+ size_t count = PAGE_CACHE_SIZE;
+ u64 attr_ver = 0;
+ int err;
+
+ err = -EIO;
+ if (is_bad_inode(inode))
+ goto out;
+
+ num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver);
if (!err) {
/*
* Short read means EOF. If file size is larger, truncate it
@@ -747,10 +765,11 @@ static int fuse_readpage(struct file *file, struct page *page)
SetPageUptodate(page);
}
-
- fuse_put_request(fc, req);
- fuse_invalidate_attr(inode); /* atime changed */
- out:
+ if (req) {
+ fuse_put_request(fc, req);
+ fuse_invalidate_attr(inode); /* atime changed */
+ }
+out:
unlock_page(page);
return err;
}
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 07/11] fuse: restructure fuse_readpage()
2013-10-10 13:11 ` [PATCH 07/11] fuse: restructure fuse_readpage() Maxim Patlasov
@ 2013-11-12 17:17 ` Miklos Szeredi
2013-12-20 14:54 ` Maxim Patlasov
0 siblings, 1 reply; 23+ messages in thread
From: Miklos Szeredi @ 2013-11-12 17:17 UTC (permalink / raw)
To: Maxim Patlasov
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
On Thu, Oct 10, 2013 at 05:11:25PM +0400, Maxim Patlasov wrote:
> Move the code filling and sending read request to a separate function. Future
> patches will use it for .write_begin -- partial modification of a page
> requires reading the page from the storage very similarly to what fuse_readpage
> does.
>
> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
> ---
> fs/fuse/file.c | 55 +++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 37 insertions(+), 18 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index b4d4189..77eb849 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -700,21 +700,14 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
> }
> }
>
> -static int fuse_readpage(struct file *file, struct page *page)
> +static int __fuse_readpage(struct file *file, struct page *page, size_t count,
> + int *err, struct fuse_req **req_pp, u64 *attr_ver_p)
Signature of this helper looks really ugly. A quick look tells me that neither
caller actually needs 'req'. And fuse_get_attr_version() can be moved to the
one caller that needs it. And negative err can be returned. And then all those
ugly pointer args are gone and the whole thing is much simpler.
Thanks,
Miklos
> {
> struct fuse_io_priv io = { .async = 0, .file = file };
> struct inode *inode = page->mapping->host;
> struct fuse_conn *fc = get_fuse_conn(inode);
> struct fuse_req *req;
> size_t num_read;
> - loff_t pos = page_offset(page);
> - size_t count = PAGE_CACHE_SIZE;
> - u64 attr_ver;
> - int err;
> -
> - err = -EIO;
> - if (is_bad_inode(inode))
> - goto out;
>
> /*
> * Page writeback can extend beyond the lifetime of the
> @@ -724,20 +717,45 @@ static int fuse_readpage(struct file *file, struct page *page)
> fuse_wait_on_page_writeback(inode, page->index);
>
> req = fuse_get_req(fc, 1);
> - err = PTR_ERR(req);
> + *err = PTR_ERR(req);
> if (IS_ERR(req))
> - goto out;
> + return 0;
>
> - attr_ver = fuse_get_attr_version(fc);
> + if (attr_ver_p)
> + *attr_ver_p = fuse_get_attr_version(fc);
>
> req->out.page_zeroing = 1;
> req->out.argpages = 1;
> req->num_pages = 1;
> req->pages[0] = page;
> req->page_descs[0].length = count;
> - num_read = fuse_send_read(req, &io, pos, count, NULL);
> - err = req->out.h.error;
>
> + num_read = fuse_send_read(req, &io, page_offset(page), count, NULL);
> + *err = req->out.h.error;
> +
> + if (*err)
> + fuse_put_request(fc, req);
> + else
> + *req_pp = req;
> +
> + return num_read;
> +}
> +
> +static int fuse_readpage(struct file *file, struct page *page)
> +{
> + struct inode *inode = page->mapping->host;
> + struct fuse_conn *fc = get_fuse_conn(inode);
> + struct fuse_req *req = NULL;
> + size_t num_read;
> + size_t count = PAGE_CACHE_SIZE;
> + u64 attr_ver = 0;
> + int err;
> +
> + err = -EIO;
> + if (is_bad_inode(inode))
> + goto out;
> +
> + num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver);
> if (!err) {
> /*
> * Short read means EOF. If file size is larger, truncate it
> @@ -747,10 +765,11 @@ static int fuse_readpage(struct file *file, struct page *page)
>
> SetPageUptodate(page);
> }
> -
> - fuse_put_request(fc, req);
> - fuse_invalidate_attr(inode); /* atime changed */
> - out:
> + if (req) {
> + fuse_put_request(fc, req);
> + fuse_invalidate_attr(inode); /* atime changed */
> + }
> +out:
> unlock_page(page);
> return err;
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 07/11] fuse: restructure fuse_readpage()
2013-11-12 17:17 ` Miklos Szeredi
@ 2013-12-20 14:54 ` Maxim Patlasov
2014-01-06 16:43 ` Miklos Szeredi
0 siblings, 1 reply; 23+ messages in thread
From: Maxim Patlasov @ 2013-12-20 14:54 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
Hi Miklos,
Sorry for delay, see please inline comments below.
On 11/12/2013 09:17 PM, Miklos Szeredi wrote:
> On Thu, Oct 10, 2013 at 05:11:25PM +0400, Maxim Patlasov wrote:
>> Move the code filling and sending read request to a separate function. Future
>> patches will use it for .write_begin -- partial modification of a page
>> requires reading the page from the storage very similarly to what fuse_readpage
>> does.
>>
>> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
>> ---
>> fs/fuse/file.c | 55 +++++++++++++++++++++++++++++++++++++------------------
>> 1 file changed, 37 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index b4d4189..77eb849 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -700,21 +700,14 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
>> }
>> }
>>
>> -static int fuse_readpage(struct file *file, struct page *page)
>> +static int __fuse_readpage(struct file *file, struct page *page, size_t count,
>> + int *err, struct fuse_req **req_pp, u64 *attr_ver_p)
> Signature of this helper looks really ugly. A quick look tells me that neither
> caller actually needs 'req'.
fuse_readpage() passes 'req' to fuse_short_read(). And the latter uses
req->pages[] to nullify a part of request.
> And fuse_get_attr_version() can be moved to the
> one caller that needs it.
Yes, it's doable. But this would make attr_version mechanism less
efficient (under some loads): suppose the file on server was truncated
externally, then fuse_readpage() acquires fc->attr_version, then some
innocent write bumps fc->attr_version while we're waiting for fuse
writeback, then fuse_read_update_size() would noop. In the other words,
it's beneficial to keep the time interval between acquiring
fc->attr_version and subsequent comparison as short as possible.
> And negative err can be returned.
Yes, but this will require some precautions for positive
req->out.h.error. Like "err = (req->out.h.error <= 0) ? req->out.h.error
: -EIO;". But this must be OK - filtering out positive req->out.h.error
is a good idea, imho.
> And then all those
> ugly pointer args are gone and the whole thing is much simpler.
If you agree with my comments above, only 1 of 3 ugly pointers can be
avoided. Another way would be to revert the code back to the initial
implementation where fuse_readpage() and fuse_prepare_write() sent read
requests independently.
Thanks,
Maxim
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 07/11] fuse: restructure fuse_readpage()
2013-12-20 14:54 ` Maxim Patlasov
@ 2014-01-06 16:43 ` Miklos Szeredi
2014-01-20 11:46 ` Maxim Patlasov
0 siblings, 1 reply; 23+ messages in thread
From: Miklos Szeredi @ 2014-01-06 16:43 UTC (permalink / raw)
To: Maxim Patlasov
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
On Fri, Dec 20, 2013 at 06:54:40PM +0400, Maxim Patlasov wrote:
> Hi Miklos,
>
> Sorry for delay, see please inline comments below.
>
> On 11/12/2013 09:17 PM, Miklos Szeredi wrote:
> >On Thu, Oct 10, 2013 at 05:11:25PM +0400, Maxim Patlasov wrote:
> >>Move the code filling and sending read request to a separate function. Future
> >>patches will use it for .write_begin -- partial modification of a page
> >>requires reading the page from the storage very similarly to what fuse_readpage
> >>does.
> >>
> >>Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
> >>---
> >> fs/fuse/file.c | 55 +++++++++++++++++++++++++++++++++++++------------------
> >> 1 file changed, 37 insertions(+), 18 deletions(-)
> >>
> >>diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> >>index b4d4189..77eb849 100644
> >>--- a/fs/fuse/file.c
> >>+++ b/fs/fuse/file.c
> >>@@ -700,21 +700,14 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
> >> }
> >> }
> >>-static int fuse_readpage(struct file *file, struct page *page)
> >>+static int __fuse_readpage(struct file *file, struct page *page, size_t count,
> >>+ int *err, struct fuse_req **req_pp, u64 *attr_ver_p)
> >Signature of this helper looks really ugly. A quick look tells me that neither
> >caller actually needs 'req'.
>
> fuse_readpage() passes 'req' to fuse_short_read(). And the latter
> uses req->pages[] to nullify a part of request.
I don't get it. __fuse_readpage() itself call's fuse_short_read(), not callers
of __fuse_readpage(). Or do they?
>
> >And fuse_get_attr_version() can be moved to the
> >one caller that needs it.
>
> Yes, it's doable. But this would make attr_version mechanism less
> efficient (under some loads): suppose the file on server was
> truncated externally, then fuse_readpage() acquires
> fc->attr_version, then some innocent write bumps fc->attr_version
> while we're waiting for fuse writeback, then fuse_read_update_size()
> would noop. In the other words, it's beneficial to keep the time
> interval between acquiring fc->attr_version and subsequent
> comparison as short as possible.
Okay, lets try to keep this the way it is. I don't like it very much, but I
fear changing user visible behavior.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 07/11] fuse: restructure fuse_readpage()
2014-01-06 16:43 ` Miklos Szeredi
@ 2014-01-20 11:46 ` Maxim Patlasov
0 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2014-01-20 11:46 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
On 01/06/2014 08:43 PM, Miklos Szeredi wrote:
> On Fri, Dec 20, 2013 at 06:54:40PM +0400, Maxim Patlasov wrote:
>> Hi Miklos,
>>
>> Sorry for delay, see please inline comments below.
>>
>> On 11/12/2013 09:17 PM, Miklos Szeredi wrote:
>>> On Thu, Oct 10, 2013 at 05:11:25PM +0400, Maxim Patlasov wrote:
>>>> Move the code filling and sending read request to a separate function. Future
>>>> patches will use it for .write_begin -- partial modification of a page
>>>> requires reading the page from the storage very similarly to what fuse_readpage
>>>> does.
>>>>
>>>> Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
>>>> ---
>>>> fs/fuse/file.c | 55 +++++++++++++++++++++++++++++++++++++------------------
>>>> 1 file changed, 37 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>>> index b4d4189..77eb849 100644
>>>> --- a/fs/fuse/file.c
>>>> +++ b/fs/fuse/file.c
>>>> @@ -700,21 +700,14 @@ static void fuse_short_read(struct fuse_req *req, struct inode *inode,
>>>> }
>>>> }
>>>> -static int fuse_readpage(struct file *file, struct page *page)
>>>> +static int __fuse_readpage(struct file *file, struct page *page, size_t count,
>>>> + int *err, struct fuse_req **req_pp, u64 *attr_ver_p)
>>> Signature of this helper looks really ugly. A quick look tells me that neither
>>> caller actually needs 'req'.
>> fuse_readpage() passes 'req' to fuse_short_read(). And the latter
>> uses req->pages[] to nullify a part of request.
> I don't get it. __fuse_readpage() itself call's fuse_short_read(), not callers
> of __fuse_readpage(). Or do they?
fuse_readpage() is a caller of __fuse_readpage() and it looks (after
applying the patch) like this:
> static int fuse_readpage(struct file *file, struct page *page)
> {
> ...
> num_read = __fuse_readpage(file, page, count, &err, &req, &attr_ver);
> if (!err) {
> /*
> * Short read means EOF. If file size is larger, truncate it
> */
> if (num_read < count)
> fuse_short_read(req, inode, attr_ver);
>
> SetPageUptodate(page);
> }
Thanks,
Maxim
>
>>> And fuse_get_attr_version() can be moved to the
>>> one caller that needs it.
>> Yes, it's doable. But this would make attr_version mechanism less
>> efficient (under some loads): suppose the file on server was
>> truncated externally, then fuse_readpage() acquires
>> fc->attr_version, then some innocent write bumps fc->attr_version
>> while we're waiting for fuse writeback, then fuse_read_update_size()
>> would noop. In the other words, it's beneficial to keep the time
>> interval between acquiring fc->attr_version and subsequent
>> comparison as short as possible.
> Okay, lets try to keep this the way it is. I don't like it very much, but I
> fear changing user visible behavior.
>
> Thanks,
> Miklos
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 08/11] fuse: Implement write_begin/write_end callbacks -v2
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (6 preceding siblings ...)
2013-10-10 13:11 ` [PATCH 07/11] fuse: restructure fuse_readpage() Maxim Patlasov
@ 2013-10-10 13:11 ` Maxim Patlasov
2013-10-10 13:11 ` [PATCH 09/11] fuse: fuse_flush() should wait on writeback Maxim Patlasov
` (3 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:11 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
The .write_begin and .write_end are requiered to use generic routines
(generic_file_aio_write --> ... --> generic_perform_write) for buffered
writes.
Changed in v2:
- added fuse_wait_on_page_writeback() to fuse ->write_begin; this is
crucial to avoid the lost of user data by incorrect crop of a secondary
request to a stale inarg->size of the primary.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/file.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 100 insertions(+)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 77eb849..642bd51 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1950,6 +1950,104 @@ out:
return err;
}
+/*
+ * Determine the number of bytes of data the page contains
+ */
+static inline unsigned fuse_page_length(struct page *page)
+{
+ loff_t i_size = i_size_read(page_file_mapping(page)->host);
+
+ if (i_size > 0) {
+ pgoff_t page_index = page_file_index(page);
+ pgoff_t end_index = (i_size - 1) >> PAGE_CACHE_SHIFT;
+ if (page_index < end_index)
+ return PAGE_CACHE_SIZE;
+ if (page_index == end_index)
+ return ((i_size - 1) & ~PAGE_CACHE_MASK) + 1;
+ }
+ return 0;
+}
+
+static int fuse_prepare_write(struct fuse_conn *fc, struct file *file,
+ struct page *page, loff_t pos, unsigned len)
+{
+ struct fuse_req *req = NULL;
+ unsigned num_read;
+ unsigned page_len;
+ int err;
+
+ if (PageUptodate(page) || (len == PAGE_CACHE_SIZE)) {
+ fuse_wait_on_page_writeback(page->mapping->host, page->index);
+ return 0;
+ }
+
+ page_len = fuse_page_length(page);
+ if (!page_len) {
+ fuse_wait_on_page_writeback(page->mapping->host, page->index);
+ zero_user(page, 0, PAGE_CACHE_SIZE);
+ return 0;
+ }
+
+ num_read = __fuse_readpage(file, page, page_len, &err, &req, NULL);
+ if (req)
+ fuse_put_request(fc, req);
+ if (err) {
+ unlock_page(page);
+ page_cache_release(page);
+ } else if (num_read != PAGE_CACHE_SIZE) {
+ zero_user_segment(page, num_read, PAGE_CACHE_SIZE);
+ }
+ return err;
+}
+
+/*
+ * It's worthy to make sure that space is reserved on disk for the write,
+ * but how to implement it without killing performance need more thinking.
+ */
+static int fuse_write_begin(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned flags,
+ struct page **pagep, void **fsdata)
+{
+ pgoff_t index = pos >> PAGE_CACHE_SHIFT;
+ struct fuse_conn *fc = get_fuse_conn(file->f_dentry->d_inode);
+
+ BUG_ON(!fc->writeback_cache);
+
+ *pagep = grab_cache_page_write_begin(mapping, index, flags);
+ if (!*pagep)
+ return -ENOMEM;
+
+ return fuse_prepare_write(fc, file, *pagep, pos, len);
+}
+
+static int fuse_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;
+
+ if (!PageUptodate(page))
+ SetPageUptodate(page);
+
+ fuse_write_update_size(inode, pos);
+ set_page_dirty(page);
+ return 0;
+}
+
+static int fuse_write_end(struct file *file, struct address_space *mapping,
+ loff_t pos, unsigned len, unsigned copied,
+ struct page *page, void *fsdata)
+{
+ unsigned from = pos & (PAGE_CACHE_SIZE - 1);
+
+ fuse_commit_write(file, page, from, from+copied);
+
+ unlock_page(page);
+ page_cache_release(page);
+
+ return copied;
+}
+
static int fuse_launder_page(struct page *page)
{
int err = 0;
@@ -2974,6 +3072,8 @@ static const struct address_space_operations fuse_file_aops = {
.set_page_dirty = __set_page_dirty_nobuffers,
.bmap = fuse_bmap,
.direct_IO = fuse_direct_IO,
+ .write_begin = fuse_write_begin,
+ .write_end = fuse_write_end,
};
void fuse_init_file_inode(struct inode *inode)
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 09/11] fuse: fuse_flush() should wait on writeback
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (7 preceding siblings ...)
2013-10-10 13:11 ` [PATCH 08/11] fuse: Implement write_begin/write_end callbacks -v2 Maxim Patlasov
@ 2013-10-10 13:11 ` Maxim Patlasov
2013-10-10 13:12 ` [PATCH 10/11] fuse: Fix O_DIRECT operations vs cached writeback misorder - v2 Maxim Patlasov
` (2 subsequent siblings)
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:11 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
The aim of .flush fop is to hint file-system that flushing its state or caches
or any other important data to reliable storage would be desirable now.
fuse_flush() passes this hint by sending FUSE_FLUSH request to userspace.
However, dirty pages and pages under writeback may be not visible to userspace
yet if we won't ensure it explicitly.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/file.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 642bd51..f23fc65 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -19,6 +19,7 @@
#include <linux/falloc.h>
static const struct file_operations fuse_direct_io_file_operations;
+static void fuse_sync_writes(struct inode *inode);
static int fuse_send_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
int opcode, struct fuse_open_out *outargp)
@@ -399,6 +400,14 @@ static int fuse_flush(struct file *file, fl_owner_t id)
if (fc->no_flush)
return 0;
+ err = filemap_write_and_wait(file->f_mapping);
+ if (err)
+ return err;
+
+ mutex_lock(&inode->i_mutex);
+ fuse_sync_writes(inode);
+ mutex_unlock(&inode->i_mutex);
+
req = fuse_get_req_nofail_nopages(fc, file);
memset(&inarg, 0, sizeof(inarg));
inarg.fh = ff->fh;
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 10/11] fuse: Fix O_DIRECT operations vs cached writeback misorder - v2
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (8 preceding siblings ...)
2013-10-10 13:11 ` [PATCH 09/11] fuse: fuse_flush() should wait on writeback Maxim Patlasov
@ 2013-10-10 13:12 ` Maxim Patlasov
2013-10-10 13:12 ` [PATCH 11/11] fuse: Turn writeback cache on Maxim Patlasov
[not found] ` <87li13pido.fsf@vostro.rath.org>
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:12 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
The problem is:
1. write cached data to a file
2. read directly from the same file (via another fd)
The 2nd operation may read stale data, i.e. the one that was in a file
before the 1st op. Problem is in how fuse manages writeback.
When direct op occurs the core kernel code calls filemap_write_and_wait
to flush all the cached ops in flight. But fuse acks the writeback right
after the ->writepages callback exits w/o waiting for the real write to
happen. Thus the subsequent direct op proceeds while the real writeback
is still in flight. This is a problem for backends that reorder operation.
Fix this by making the fuse direct IO callback explicitly wait on the
in-flight writeback to finish.
Changed in v2:
- do not wait on writeback if fuse_direct_io() call came from
CUSE (because it doesn't use fuse inodes)
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/cuse.c | 5 +++--
fs/fuse/file.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
fs/fuse/fuse_i.h | 13 ++++++++++++-
3 files changed, 62 insertions(+), 5 deletions(-)
diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
index adbfd66..9bf4bb2 100644
--- a/fs/fuse/cuse.c
+++ b/fs/fuse/cuse.c
@@ -95,7 +95,7 @@ static ssize_t cuse_read(struct file *file, char __user *buf, size_t count,
struct iovec iov = { .iov_base = buf, .iov_len = count };
struct fuse_io_priv io = { .async = 0, .file = file };
- return fuse_direct_io(&io, &iov, 1, count, &pos, 0);
+ return fuse_direct_io(&io, &iov, 1, count, &pos, FUSE_DIO_CUSE);
}
static ssize_t cuse_write(struct file *file, const char __user *buf,
@@ -109,7 +109,8 @@ static ssize_t cuse_write(struct file *file, const char __user *buf,
* No locking or generic_write_checks(), the server is
* responsible for locking and sanity checks.
*/
- return fuse_direct_io(&io, &iov, 1, count, &pos, 1);
+ return fuse_direct_io(&io, &iov, 1, count, &pos,
+ FUSE_DIO_WRITE | FUSE_DIO_CUSE);
}
static int cuse_open(struct inode *inode, struct file *file)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index f23fc65..653ae9c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -341,6 +341,31 @@ u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id)
return (u64) v0 + ((u64) v1 << 32);
}
+static bool fuse_range_is_writeback(struct inode *inode, pgoff_t idx_from,
+ pgoff_t idx_to)
+{
+ struct fuse_conn *fc = get_fuse_conn(inode);
+ struct fuse_inode *fi = get_fuse_inode(inode);
+ struct fuse_req *req;
+ bool found = false;
+
+ spin_lock(&fc->lock);
+ list_for_each_entry(req, &fi->writepages, writepages_entry) {
+ pgoff_t curr_index;
+
+ BUG_ON(req->inode != inode);
+ curr_index = req->misc.write.in.offset >> PAGE_CACHE_SHIFT;
+ if (!(idx_from >= curr_index + req->num_pages ||
+ idx_to < curr_index)) {
+ found = true;
+ break;
+ }
+ }
+ spin_unlock(&fc->lock);
+
+ return found;
+}
+
/*
* Check if page is under writeback
*
@@ -385,6 +410,19 @@ static int fuse_wait_on_page_writeback(struct inode *inode, pgoff_t index)
return 0;
}
+static void fuse_wait_on_writeback(struct inode *inode, pgoff_t start,
+ size_t bytes)
+{
+ struct fuse_inode *fi = get_fuse_inode(inode);
+ pgoff_t idx_from, idx_to;
+
+ idx_from = start >> PAGE_CACHE_SHIFT;
+ idx_to = (start + bytes - 1) >> PAGE_CACHE_SHIFT;
+
+ wait_event(fi->page_waitq,
+ !fuse_range_is_writeback(inode, idx_from, idx_to));
+}
+
static int fuse_flush(struct file *file, fl_owner_t id)
{
struct inode *inode = file_inode(file);
@@ -1363,8 +1401,10 @@ static inline int fuse_iter_npages(const struct iov_iter *ii_p)
ssize_t fuse_direct_io(struct fuse_io_priv *io, const struct iovec *iov,
unsigned long nr_segs, size_t count, loff_t *ppos,
- int write)
+ int flags)
{
+ int write = flags & FUSE_DIO_WRITE;
+ int cuse = flags & FUSE_DIO_CUSE;
struct file *file = io->file;
struct fuse_file *ff = file->private_data;
struct fuse_conn *fc = ff->fc;
@@ -1393,6 +1433,10 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, const struct iovec *iov,
break;
}
+ if (!cuse)
+ fuse_wait_on_writeback(file->f_mapping->host, pos,
+ nbytes);
+
if (write)
nres = fuse_send_write(req, io, pos, nbytes, owner);
else
@@ -1471,7 +1515,8 @@ static ssize_t __fuse_direct_write(struct fuse_io_priv *io,
res = generic_write_checks(file, ppos, &count, 0);
if (!res)
- res = fuse_direct_io(io, iov, nr_segs, count, ppos, 1);
+ res = fuse_direct_io(io, iov, nr_segs, count, ppos,
+ FUSE_DIO_WRITE);
fuse_invalidate_attr(inode);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 3028b8a..129af78 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -862,9 +862,20 @@ int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
bool isdir);
+
+/**
+ * fuse_direct_io() flags
+ */
+
+/** If set, it is WRITE; otherwise - READ */
+#define FUSE_DIO_WRITE (1 << 0)
+
+/** CUSE pass fuse_direct_io() a file which f_mapping->host is not from FUSE */
+#define FUSE_DIO_CUSE (1 << 1)
+
ssize_t fuse_direct_io(struct fuse_io_priv *io, const struct iovec *iov,
unsigned long nr_segs, size_t count, loff_t *ppos,
- int write);
+ int flags);
long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
unsigned int flags);
long fuse_ioctl_common(struct file *file, unsigned int cmd,
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH 11/11] fuse: Turn writeback cache on
2013-10-10 13:09 [PATCH v6 00/11] fuse: An attempt to implement a write-back cache policy Maxim Patlasov
` (9 preceding siblings ...)
2013-10-10 13:12 ` [PATCH 10/11] fuse: Fix O_DIRECT operations vs cached writeback misorder - v2 Maxim Patlasov
@ 2013-10-10 13:12 ` Maxim Patlasov
[not found] ` <87li13pido.fsf@vostro.rath.org>
11 siblings, 0 replies; 23+ messages in thread
From: Maxim Patlasov @ 2013-10-10 13:12 UTC (permalink / raw)
To: miklos
Cc: dev, xemul, fuse-devel, bfoster, linux-kernel, jbottomley,
linux-fsdevel, akpm, fengguang.wu, devel
From: Pavel Emelyanov <xemul@openvz.org>
Introduce a bit kernel and userspace exchange between each-other on
the init stage and turn writeback on if the userspace want this and
mount option 'allow_wbcache' is present (controlled by fusermount).
Also add each writable file into per-inode write list and call the
generic_file_aio_write to make use of the Linux page cache engine.
Signed-off-by: Maxim Patlasov <MPatlasov@parallels.com>
---
fs/fuse/file.c | 5 +++++
fs/fuse/fuse_i.h | 4 ++++
fs/fuse/inode.c | 13 +++++++++++++
include/uapi/linux/fuse.h | 2 ++
4 files changed, 24 insertions(+)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 653ae9c..aa38e77 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -208,6 +208,8 @@ void fuse_finish_open(struct inode *inode, struct file *file)
spin_unlock(&fc->lock);
fuse_invalidate_attr(inode);
}
+ if ((file->f_mode & FMODE_WRITE) && fc->writeback_cache)
+ fuse_link_write_file(file);
}
int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
@@ -1228,6 +1230,9 @@ static ssize_t fuse_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
struct iov_iter i;
loff_t endbyte = 0;
+ if (get_fuse_conn(inode)->writeback_cache)
+ return generic_file_aio_write(iocb, iov, nr_segs, pos);
+
WARN_ON(iocb->ki_pos != pos);
ocount = 0;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 129af78..76c4f77 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -44,6 +44,10 @@
doing the mount will be allowed to access the filesystem */
#define FUSE_ALLOW_OTHER (1 << 1)
+/** If the FUSE_ALLOW_WBCACHE flag is given, the filesystem
+ module will enable support of writback cache */
+#define FUSE_ALLOW_WBCACHE (1 << 2)
+
/** Number of page pointers embedded in fuse_req */
#define FUSE_REQ_INLINE_PAGES 1
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 253701f..6ddf135 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -460,6 +460,7 @@ enum {
OPT_ALLOW_OTHER,
OPT_MAX_READ,
OPT_BLKSIZE,
+ OPT_ALLOW_WBCACHE,
OPT_ERR
};
@@ -472,6 +473,7 @@ static const match_table_t tokens = {
{OPT_ALLOW_OTHER, "allow_other"},
{OPT_MAX_READ, "max_read=%u"},
{OPT_BLKSIZE, "blksize=%u"},
+ {OPT_ALLOW_WBCACHE, "allow_wbcache"},
{OPT_ERR, NULL}
};
@@ -545,6 +547,10 @@ static int parse_fuse_opt(char *opt, struct fuse_mount_data *d, int is_bdev)
d->blksize = value;
break;
+ case OPT_ALLOW_WBCACHE:
+ d->flags |= FUSE_ALLOW_WBCACHE;
+ break;
+
default:
return 0;
}
@@ -572,6 +578,8 @@ static int fuse_show_options(struct seq_file *m, struct dentry *root)
seq_printf(m, ",max_read=%u", fc->max_read);
if (sb->s_bdev && sb->s_blocksize != FUSE_DEFAULT_BLKSIZE)
seq_printf(m, ",blksize=%lu", sb->s_blocksize);
+ if (fc->flags & FUSE_ALLOW_WBCACHE)
+ seq_puts(m, ",allow_wbcache");
return 0;
}
@@ -889,6 +897,9 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_req *req)
}
if (arg->flags & FUSE_ASYNC_DIO)
fc->async_dio = 1;
+ if (arg->flags & FUSE_WRITEBACK_CACHE &&
+ fc->flags & FUSE_ALLOW_WBCACHE)
+ fc->writeback_cache = 1;
} else {
ra_pages = fc->max_read / PAGE_CACHE_SIZE;
fc->no_lock = 1;
@@ -917,6 +928,8 @@ static void fuse_send_init(struct fuse_conn *fc, struct fuse_req *req)
FUSE_SPLICE_WRITE | FUSE_SPLICE_MOVE | FUSE_SPLICE_READ |
FUSE_FLOCK_LOCKS | FUSE_IOCTL_DIR | FUSE_AUTO_INVAL_DATA |
FUSE_DO_READDIRPLUS | FUSE_READDIRPLUS_AUTO | FUSE_ASYNC_DIO;
+ if (fc->flags & FUSE_ALLOW_WBCACHE)
+ arg->flags |= FUSE_WRITEBACK_CACHE;
req->in.h.opcode = FUSE_INIT;
req->in.numargs = 1;
req->in.args[0].size = sizeof(*arg);
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 60bb2f9..5c98dd1 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -219,6 +219,7 @@ struct fuse_file_lock {
* FUSE_DO_READDIRPLUS: do READDIRPLUS (READDIR+LOOKUP in one)
* FUSE_READDIRPLUS_AUTO: adaptive readdirplus
* FUSE_ASYNC_DIO: asynchronous direct I/O submission
+ * FUSE_WRITEBACK_CACHE: use writeback cache for buffered writes
*/
#define FUSE_ASYNC_READ (1 << 0)
#define FUSE_POSIX_LOCKS (1 << 1)
@@ -236,6 +237,7 @@ struct fuse_file_lock {
#define FUSE_DO_READDIRPLUS (1 << 13)
#define FUSE_READDIRPLUS_AUTO (1 << 14)
#define FUSE_ASYNC_DIO (1 << 15)
+#define FUSE_WRITEBACK_CACHE (1 << 16)
/**
* CUSE INIT request/reply flags
^ permalink raw reply related [flat|nested] 23+ messages in thread[parent not found: <87li13pido.fsf@vostro.rath.org>]