fuse: An attempt to implement a write-back cache policy

The .writepages callback will issue writeback requests with more than one page aboard. Make existing end/check code be aware of this.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 fs/fuse/file.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

fs/fuse/file.c +0 -0

index 6bf9723..47f0f2e 100644

Quoted 14 lines · Pavel Emelyanov

The .writepages callback will issue writeback requests with more than one page aboard. Make existing end/check code be aware of this.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 fs/fuse/file.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

fs/fuse/file.c +0 -0

index 6bf9723..47f0f2e 100644
@@ -345,7 +345,7 @@ static bool fuse_page_is_writeback(struct inode *inode, pgoff_t index)
 

fs/fuse/file.c static bool fuse_page_is_writeback(struct inode *inode, pgoff_t index) · line 348

Quoted 4 lines

fs/fuse/file.c +1 -1

 		BUG_ON(req->inode != inode);
 		curr_index = req->misc.write.in.offset >> PAGE_CACHE_SHIFT;
-		if (curr_index == index) {
+		if (curr_index == index && index < curr_index + req->num_pages) {

This condition looks bogus.

Thanks,
Miklos
.

Hi everyone.

One of the problems with the existing FUSE implementation is that it uses the write-through cache policy which results in performance problems on certain workloads. E.g. when copying a big file into a FUSE file the cp pushes every 128k to the userspace synchronously. This becomes a problem when the userspace back-end uses networking for storing the data.

A good solution of this is switching the FUSE page cache into a write-back policy. With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?)

When the writeback is turned ON:

* still copy writeback pages to temporary buffer when sending a writeback request and finish the page writeback immediately

Hi Pavel,

I think it's great that you're working on this! I've been waiting for FUSE being able to supply write data in bigger chunks for a long time, and I'm very excited to see some progress on this. I'm not a kernel developer, but I'll be happy to try the patches.

While I try to get this to compile,

a few more questions:

Quoted 13 lines · Pavel Emelyanov

Hi everyone.

One of the problems with the existing FUSE implementation is that it uses the write-through cache policy which results in performance problems on certain workloads. E.g. when copying a big file into a FUSE file the cp pushes every 128k to the userspace synchronously. This becomes a problem when the userspace back-end uses networking for storing the data.

A good solution of this is switching the FUSE page cache into a write-back policy. With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

Quoted 2 lines

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?)

From your description it sounds as if the only effect of write-back is to increase the chunk size. Why is the a need to require special privileges for this?

Quoted 4 lines

When the writeback is turned ON:

* still copy writeback pages to temporary buffer when sending a writeback request and finish the page writeback immediately

Could you elaborate? I don't understand what you're saying here.

Best,

-Nikolaus

Quoted 6 lines · Nikolaus Rath

Hi Pavel,

I think it's great that you're working on this! I've been waiting for FUSE being able to supply write data in bigger chunks for a long time, and I'm very excited to see some progress on this. I'm not a kernel developer, but I'll be happy to try the patches.

Just to make it clear. I didn't increase the 32 pages per request limit. What I did is made FUSE submit more than one request at a time while serving massive writes. So yes, bigger chunks can be now seen by the daemon, but it should read several requests for that.

Quoted 8 lines

Quoted 7 lines

A good solution of this is switching the FUSE page cache into a write-back policy. With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?)

Quoted 3 lines

From your description it sounds as if the only effect of write-back is to increase the chunk size. Why the need to require special privileges for this?

Provided I understand the code correctly: if FUSE daemon turns writeback on and sets per-bdi dirty limit too high it can cause a deadlock on the box. Thus then daemon should be trusted by the kernel, i.e. -- privileged.

Quoted 8 lines · Pavel Emelyanov

Quoted 6 lines · Nikolaus Rath

Hi Pavel,

I think it's great that you're working on this! I've been waiting for FUSE being able to supply write data in bigger chunks for a long time, and I'm very excited to see some progress on this. I'm not a kernel developer, but I'll be happy to try the patches.

Quoted 4 lines

Just to make it clear. I didn't increase the 32 pages per request limit. What I did is made FUSE submit more than one request at a time while serving massive writes. So yes, bigger chunks can be now seen by the daemon, but it should read several requests for that.

Ah, I thought that your patch would do both. So with the patch an userspace client can now writes data in say 4 kb chunks, and the FUSE daemon will still receive it from the kernel in 128 kb chunks?

But if the client writes a say 1 MB chunk, the FUSE daemon will still see 8 128kb write requests?

Would it be very hard to raise the 32 pages per request limit at the same time?

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

A good solution of this is switching the FUSE page cache into a write-back policy. With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?)

From your description it sounds as if the only effect of write-back is to increase the chunk size. Why the need to require special privileges for this?

Quoted 3 lines

Provided I understand the code correctly: if FUSE daemon turns writeback on and sets per-bdi dirty limit too high it can cause a deadlock on the box. Thus then daemon should be trusted by the kernel, i.e. -- privileged.

Wouldn't it be more reasonable to enforce that the bdi dirty limit is not set too high then?

Thanks,

-Nikolaus

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 15 lines

A good solution of this is switching the FUSE page cache into a write-back policy.

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Quoted 6 lines

With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?)

From your description it sounds as if the only effect of write-back is to increase the chunk size. Why the need to require special privileges for this?

Provided I understand the code correctly: if FUSE daemon turns writeback on and sets per-bdi dirty limit too high it can cause a deadlock on the box. Thus then daemon should be trusted by the kernel, i.e. -- privileged.

Quoted 2 lines

Quoted 2 lines

Wouldn't it be more reasonable to enforce that the bdi dirty limit is not set too high then?

Hardly, since if you have several mounts with small bdi limit each the sum of them can be still high.

Quoted 16 lines · Pavel Emelyanov

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 15 lines

A good solution of this is switching the FUSE page cache into a write-back policy.

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Quoted 6 lines

With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?)

From your description it sounds as if the only effect of write-back is to increase the chunk size. Why the need to require special privileges for this?

Provided I understand the code correctly: if FUSE daemon turns writeback on and sets per-bdi dirty limit too high it can cause a deadlock on the box. Thus then daemon should be trusted by the kernel, i.e. -- privileged.

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Wouldn't it be more reasonable to enforce that the bdi dirty limit is not set too high then?

Hardly, since if you have several mounts with small bdi limit each the sum of them can be still high.

Yeah, so isn't it possible to put a limit on the sum of them?

I think it would be much better if this could be made safe to use instead of requiring special privileges. If that isn't possible, wouldn't it be better to enable/disable the feature in /etc/fuse.conf, similar to the user_allow_* options?

Having to explain how to assign capabilities in the documentation of a fuse filesystem sounds quite dreadful to me...

Best,

-Nikolaus

PS: My naive attempt to apply your patches to my Debian 3.2 kernel failed. I'm compiling 3.5rc5 at the moment - will see if I can get it to boot afterwards...

Quoted 1 line

While I try to get this to compile,

That's good news! Thanks a lot!

Quoted 3 lines · Pavel Emelyanov

Quoted 1 line

While I try to get this to compile,

That's good news! Thanks a lot!

Ok, I got it to compile and tested it by copying a large file into an S3QL FUSE file system using different block sizes. At first glance, things look fantastic.

With kernel 3.2:

Blocksize: 4k
131072000 bytes (131 MB) copied, 7.76037 s, 16.9 MB/s

However, I suspect that most of the gain is really because with the patch most of the data is still in the kernel cache when dd finishes and hasn't yet been received by the FUSE client.

Is there a way to force flushing of the fuse cache, so that I can measure the time for dd + final flush?

Best,

-Nikolaus

Quoted 4 lines · Nikolaus Rath

Quoted 3 lines · Pavel Emelyanov

Quoted 1 line

While I try to get this to compile,

That's good news! Thanks a lot!

The fuse and fsdevel mainling lists were corrupted in you original message, so looping the lists back in.

Quoted 3 lines

Ok, I got it to compile and tested it by copying a large file into an S3QL FUSE file system using different block sizes. At first glance, things look fantastic.

That's good news actually! :)

Quoted 5 lines

With kernel 3.2:

Blocksize: 4k
131072000 bytes (131 MB) copied, 7.76037 s, 16.9 MB/s

[snip]

Quoted 6 lines

However, I suspect that most of the gain is really because with the patch most of the data is still in the kernel cache when dd finishes and hasn't yet been received by the FUSE client.

Quoted 2 lines

Is there a way to force flushing of the fuse cache, so that I can measure the time for dd + final flush?

Actually when dd closes an output file the whole page cache which relates to it gets flushed to the userspace! This is due to how FUSE write requests are served.

That said -- you've already measured writeback with flush :)

Quoted 29 lines · Pavel Emelyanov

Quoted 4 lines · Nikolaus Rath

Quoted 3 lines · Pavel Emelyanov

Quoted 1 line

While I try to get this to compile,

That's good news! Thanks a lot!

The fuse and fsdevel mainling lists were corrupted in you original message, so looping the lists back in.

Quoted 3 lines

Ok, I got it to compile and tested it by copying a large file into an S3QL FUSE file system using different block sizes. At first glance, things look fantastic.

That's good news actually! :)

Quoted 5 lines

With kernel 3.2:

Blocksize: 4k
131072000 bytes (131 MB) copied, 7.76037 s, 16.9 MB/s

[snip]

Quoted 6 lines

However, I suspect that most of the gain is really because with the patch most of the data is still in the kernel cache when dd finishes and hasn't yet been received by the FUSE client.

Quoted 8 lines

Quoted 2 lines

Is there a way to force flushing of the fuse cache, so that I can measure the time for dd + final flush?

Actually when dd closes an output file the whole page cache which relates to it gets flushed to the userspace! This is due to how FUSE write requests are served.

That said -- you've already measured writeback with flush :)

But then how is it possible that there is such a big difference even when using 128kb blocks?

Kernel 3.2:
131072000 bytes (131 MB) copied, 2.10194 s, 62.4 MB/s

Kernel 3.5-pre:
Blocksize: 128k
131072000 bytes (131 MB) copied, 0.51943 s, 252 MB/s

I would think that it both cases the FUSE daemon gets the data in 128 kb packets, so why the difference? Or is this due to some other change between 3.2 and 3.5 that significantly increased FUSE performance?

I can try the same test with the unpatched 3.5 tonight if that would be of interest.

Best,

-Nikolaus

Quoted 30 lines

Quoted 14 lines

Quoted 5 lines

With kernel 3.2:

Blocksize: 4k
131072000 bytes (131 MB) copied, 7.76037 s, 16.9 MB/s

[snip]

Quoted 6 lines

However, I suspect that most of the gain is really because with the patch most of the data is still in the kernel cache when dd finishes and hasn't yet been received by the FUSE client.

Quoted 8 lines

Quoted 2 lines

Is there a way to force flushing of the fuse cache, so that I can measure the time for dd + final flush?

Actually when dd closes an output file the whole page cache which relates to it gets flushed to the userspace! This is due to how FUSE write requests are served.

That said -- you've already measured writeback with flush :)

But then how is it possible that there is such a big difference even when using 128kb blocks?

Kernel 3.2:
131072000 bytes (131 MB) copied, 2.10194 s, 62.4 MB/s

Quoted 7 lines

Kernel 3.5-pre:
Blocksize: 128k
131072000 bytes (131 MB) copied, 0.51943 s, 252 MB/s

I would think that it both cases the FUSE daemon gets the data in 128 kb packets, so why the difference? Or is this due to some other change between 3.2 and 3.5 that significantly increased FUSE performance?

I don't know for sure. Need to read an analyze the FUSE userspace side logs (if any).

Quoted 2 lines

I can try the same test with the unpatched 3.5 tonight if that would be of interest.

Yes, this can help.

Alright, here are the resulting transfer rates for 131 MB of data:

Blocksize    Kernel 3.2         3.5-pre          3.5-pre + patch
4k           16.2               31.5             23.8
8k           52.8               48.5             91.2
16k          59.1               46.8             136
32k          64.1               47.6             185
64k          71.3               50.9             217
96k          76.1               50.6             229
128k         62.4               76.5             252
256k         66.6               68.8             286
512k         54.1               55.1             290
1024k        78.7               68.5             294

There's up to 20 MB/s variation for a block size on successive runs, but it's still obvious that the patch really makes a huge difference. Thinking about it, it actually makes sense even for the block sizes greater than 128 kb. Here the speedup probably comes from the fact that the FUSE daemon can now process several chunks at the same time.

Please let me know if there is anything I can do to help get this merged. The results look fantastic.

Best,

-Nikolaus

Hi everyone.

One of the problems with the existing FUSE implementation is that it uses the write-through cache policy which results in performance problems on certain workloads. E.g. when copying a big file into a FUSE file the cp pushes every 128k to the userspace synchronously. This becomes a problem when the userspace back-end uses networking for storing the data.

A good solution of this is switching the FUSE page cache into a write-back policy. With this file data are pushed to the userspace with big chunks (depending on the dirty memory limits, but this is much more than 128k) which lets the FUSE daemons handle the size updates in a more efficient manner.

The writeback feature is per-connection and is explicitly configurable at the init stage (is it worth making it CAP_SOMETHING protected?) When the writeback is turned ON:

* still copy writeback pages to temporary buffer when sending a writeback request and finish the page writeback immediately

* make kernel maintain the inode's i_size to avoid frequent i_size synchronization with the user space

* take NR_WRITEBACK_TEMP into account when makeing balance_dirty_pages decision. This protects us from having too many dirty pages on FUSE

The provided patchset survives the fsx test. Performance measurements are not yet all finished, but the mentioned copying of a huge file becomes noticeably faster even on machines with few RAM and doesn't make the system stuck (the dirty pages balancer does its work OK). Applies on top of v3.5-rc4.

We are currently exploring this with our own distributed storage implementation which is heavily oriented on storing big blobs of data with extremely rare meta-data updates (virtual machines' and containers' disk images). With the existing cache policy a typical usage scenario -- copying a big VM disk into a cloud -- takes way too much time to proceed, much longer than if it was simply scp-ed over the same network. The write-back policy (as I mentioned) noticeably improves this scenario. Kirill (in Cc) can share more details about the performance and the storage concepts details if required.

Thanks,
Pavel

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
fuse-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/fuse-devel

Doesn't such a change increase the "dirty debt" held in the kernel and therefore increase the probability of resulting in a deadlock (when the userspace filesystem's memory allocation request ends up waiting on a dirty page writeout caused by this write-back feature)?

Why not implement the write-back inside the userspace filesystem leaving the kernel operate in write-through, which makes things overall less unsafe?

Do you have performance numbers comparing write-back in kernel v/s write-back in userspace?

Avati

Quoted 4 lines · Anand Avati

Doesn't such a change increase the "dirty debt" held in the kernel and therefore increase the probability of resulting in a deadlock (when the userspace filesystem's memory allocation request ends up waiting on a dirty page writeout caused by this write-back feature)?

That's a very good question! I've tried to address this in the patch #10. The balance_dirty_pages start throttling task which tries to put too many dirty pages on a bdi. On FUSE this limit is set to a reasonably low value, so that the dirty set of a FUSE mount won't grow too high.

Quoted 2 lines

Why not implement the write-back inside the userspace filesystem leaving the kernel operate in write-through, which makes things overall less unsafe?

Making yet another cache in the user space is extremely hard, since the user space app has no idea at all when to start shrinking this cache. Neither it knows which part of the cache (clean/dirty) is better candidate for that. On the other hand, the in kernel cache gets shrunk when required and is balanced.

Quoted 2 lines

Do you have performance numbers comparing write-back in kernel v/s write-back in userspace?

I have only very preliminary measurements, I mostly played with the patches stability. Pushing a big portion of data (4x times bigger than RAM) from one VM to another via FUSE + networking backend with dd in 4k chunks increases the reported by dd speed more than twice.

But, Kirill Korotaev (in Cc) was supposed to perform more accurate measurements, hopefully he can provide the results.

Make fuse think that when writeback is on the inode's i_size is alway 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.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 fs/fuse/dir.c   |   10 ++++++----
 fs/fuse/file.c  |   23 +++++++++++++++++++++--
 fs/fuse/inode.c |    6 ++++--
 3 files changed, 31 insertions(+), 8 deletions(-)

fs/fuse/dir.c +6 -4

index 334e0b1..032fb20 100644
@@ -790,7 +790,7 @@ static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
 	stat->mtime.tv_nsec = attr->mtimensec;
 	stat->ctime.tv_sec = attr->ctime;
 	stat->ctime.tv_nsec = attr->ctimensec;
-	stat->size = attr->size;
+	stat->size = i_size_read(inode);
 	stat->blocks = attr->blocks;
 
 	if (attr->blksize != 0)

@@ -1350,7 +1350,7 @@ static int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
 	struct fuse_req *req;
 	struct fuse_setattr_in inarg;
 	struct fuse_attr_out outarg;
-	bool is_truncate = false;
+	bool is_truncate = false, is_wb = fc->writeback_cache;
 	loff_t oldsize;
 	int err;
 

@@ -1423,7 +1423,8 @@ static int fuse_do_setattr(struct dentry *entry, struct iattr *attr,
 	fuse_change_attributes_common(inode, &outarg.attr,
 				      attr_timeout(&outarg));
 	oldsize = inode->i_size;
-	i_size_write(inode, outarg.attr.size);
+	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 */

@@ -1435,7 +1436,8 @@ static int fuse_do_setattr(struct dentry *entry, 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, oldsize, outarg.attr.size);
 		invalidate_inode_pages2(inode->i_mapping);
 	}

fs/fuse/file.c +21 -2

index 47f0f2e..9bc1390 100644
@@ -541,9 +541,28 @@ static void fuse_read_update_size(struct inode *inode, loff_t size,
 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.
+		 */
+		int i;
+		size_t off = num_read & (PAGE_CACHE_SIZE - 1);
+
+		for (i = num_read >> PAGE_CACHE_SHIFT; i < req->num_pages; i++) {
+			struct page *page = req->pages[i];
+			void *mapaddr = kmap_atomic(page, KM_USER0);
 
-	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, KM_USER0);
+			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)

fs/fuse/inode.c +4 -2

index 1cd6165..88c577f 100644
@@ -196,6 +196,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;
 
 	spin_lock(&fc->lock);

@@ -207,10 +208,11 @@ 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);
+	if (!is_wb || !S_ISREG(inode->i_mode))
+		i_size_write(inode, attr->size);
 	spin_unlock(&fc->lock);
 
-	if (S_ISREG(inode->i_mode) && oldsize != attr->size) {
+	if (!is_wb && S_ISREG(inode->i_mode) && oldsize != attr->size) {
 		truncate_pagecache(inode, oldsize, attr->size);
 		invalidate_inode_pages2(inode->i_mapping);
 	}

Quoted 4 lines · Pavel Emelyanov

Make fuse think that when writeback is on the inode's i_size is alway 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.

Similar rule applies to i_mtime. Except it's even more tricky, since you have to flush the mtime together with the data (the flush should not update the modification time). And we have other operations which also change the mtime, and those also need to be updated.

We should probably look at what NFS is doing.

Quoted 4 lines · Pavel Emelyanov

Make fuse think that when writeback is on the inode's i_size is alway 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.

Similar rule applies to i_mtime. Except it's even more tricky, since you have to flush the mtime together with the data (the flush should not update the modification time). And we have other operations which also change the mtime, and those also need to be updated.

We should probably look at what NFS is doing.

Quoted 7 lines · Miklos Szeredi

Quoted 4 lines · Pavel Emelyanov

Make fuse think that when writeback is on the inode's i_size is alway 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.

Quoted 6 lines

Similar rule applies to i_mtime. Except it's even more tricky, since you have to flush the mtime together with the data (the flush should not update the modification time). And we have other operations which also change the mtime, and those also need to be updated.

We should probably look at what NFS is doing.

Miklos,

I've looked at how NFS and FUSE manage the i_mtime.

The NFS (if not looking at the fscache) tries to keep the i_mtime at the maximum between the local and the remote values. This looks like correct behavior for FUSE too.

But, looking at the FUSE code I see that the existing attr_version checks do implement the same approach even if we turn the writeback cache (this series) ON.

Quoted 19 lines · Pavel Emelyanov

Quoted 7 lines · Miklos Szeredi

Quoted 4 lines · Pavel Emelyanov

Make fuse think that when writeback is on the inode's i_size is alway 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.

Quoted 6 lines

Similar rule applies to i_mtime. Except it's even more tricky, since you have to flush the mtime together with the data (the flush should not update the modification time). And we have other operations which also change the mtime, and those also need to be updated.

We should probably look at what NFS is doing.

Miklos,

I've looked at how NFS and FUSE manage the i_mtime.

Quoted 7 lines

The NFS (if not looking at the fscache) tries to keep the i_mtime at the maximum between the local and the remote values. This looks like correct behavior for FUSE too.

But, looking at the FUSE code I see that the existing attr_version checks do implement the same approach even if we turn the writeback cache (this series) ON.

It's not as simple as that.

First off, fuse sets S_NOCMTIME which means the kernel won't update i_mtime on writes. Which is fine for write-through, since the time update is always the responsibility of the userspace filesystem.

If we are doing buffered writes, then the kernel must update i_mtime on write(2) and must flush that to the userspace filesystem at some point (with a SETTATTR operation).

The attr_version thing is about making sure that we don't update the kernel's cached attribute with stale data from the userspace filesystem. E.g. if a GETATTR races with a WRITE then by the time the GETATTR finishes write may have already updated i_size despite the fact that GETATTR is returning i_size before the write. In that case we don't store the i_size we believe is stale but we do use it the stat(2) reply, since it came from the userspace filesystem, which is the authoritative source of information.

But that means that if we want stat(2) to work correctly, then we must flush the updated i_mtime to userspace before we do the GETATTR.

So basically what we need is a per-inode flag that says that i_mtime has been updated (it is more recent then what userspace has) and we must update i_mtime *only* in write and not other operations which still do the mtime update in the userspace filesystem. Any operation that modifies i_mtime (and hence invalidate the attributes) must clear the flag. Any other operation which updates or invalidates the attributes must first flush the i_mtime to userspace if the flag is set.

In addition the userspace fileystem need to implement the policy similar to NFS, in which it only updates mtime if it is greater than the current one. This means that we must differentiate between an mtime update due to a buffered write from an mtime update due to an utime (and friends) system call.

I think that would work, but it's a tricky thing and needs to be thought trough.

Thanks,
Miklos
.

Quoted 9 lines

The attr_version thing is about making sure that we don't update the kernel's cached attribute with stale data from the userspace filesystem. E.g. if a GETATTR races with a WRITE then by the time the GETATTR finishes write may have already updated i_size despite the fact that GETATTR is returning i_size before the write. In that case we don't store the i_size we believe is stale but we do use it the stat(2) reply, since it came from the userspace filesystem, which is the authoritative source of information.

Quoted 2 lines

But that means that if we want stat(2) to work correctly, then we must flush the updated i_mtime to userspace before we do the GETATTR.

Why? Isn't it enough just to look at the per-inode flag (you're talking below) and take either cached i_mtime value or the user-space verion of it?

Quoted 4 lines · Pavel Emelyanov

Make fuse think that when writeback is on the inode's i_size is alway 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.

Similar rule applies to i_mtime. Except it's even more tricky, since you have to flush the mtime together with the data (the flush should not update the modification time). And we have other operations which also change the mtime, and those also need to be updated.

We should probably look at what NFS is doing.

int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)

{
    ...

    /* Flush out writes to the server in order to update c/mtime. */
    if (S_ISREG(inode->i_mode)) {
        nfs_inode_dio_wait(inode);
        err = filemap_write_and_wait(inode->i_mapping);
        if (err)
            goto out;
    }

So basically what we need is a per-inode flag that says that i_mtime has been updated (it is more recent then what userspace has) and we must update i_mtime*only* in write and not other operations which still do the mtime update in the userspace filesystem. Any operation that modifies i_mtime (and hence invalidate the attributes) must clear the flag. Any other operation which updates or invalidates the attributes must first flush the i_mtime to userspace if the flag is set.

In addition the userspace fileystem need to implement the policy similar to NFS, in which it only updates mtime if it is greater than the current one. This means that we must differentiate between an mtime update due to a buffered write from an mtime update due to an utime (and friends) system call.

If we are doing buffered writes, then the kernel must update i_mtime on write(2) and must flush that to the userspace filesystem at some point (with a SETTATTR operation).

We should probably look at what NFS is doing.

In case of NFS, the flush does updates the modification time on server. And on client, getattr triggers flush:

    int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat
    *stat)
    {
        ...

        /* Flush out writes to the server in order to update c/mtime.  */
        if (S_ISREG(inode->i_mode)) {
            nfs_inode_dio_wait(inode);
            err = filemap_write_and_wait(inode->i_mapping);
            if (err)
                goto out;
        }

In another email of this thread you suggested some approach where in-kernel fuse flushes i_mtime to userspace:

So basically what we need is a per-inode flag that says that i_mtime has been updated (it is more recent then what userspace has) and we must update i_mtime *only* in write and not other operations which still do the mtime update in the userspace filesystem. Any operation that modifies i_mtime (and hence invalidate the attributes) must clear the flag. Any other operation which updates or invalidates the attributes must first flush the i_mtime to userspace if the flag is set.

In addition the userspace fileystem need to implement the policy similar to NFS, in which it only updates mtime if it is greater than the current one. This means that we must differentiate between an mtime update due to a buffered write from an mtime update due to an utime (and friends) system call.

My question is why do we need all these complications if we could follow NFS
way: trigger flush and wait for its (and fuse write-back) completion before sending FUSE_GETATTR to userspace?

Another concern is about the idea of sending i_mtime to userspace per se. You wrote:

If we are doing buffered writes, then the kernel must update i_mtime on write(2) and must flush that to the userspace filesystem at some point (with a SETTATTR operation).

Fuse userspace may have its own non-trivial concept of 'modification time'. It's not obliged to advance its mtime on every write. The only requirement is to be consistent: if we expose new data handling READs, mtime must be advanced properly as well. But, for example, the granularity of changes is up to implementation. From this view, in-kenel fuse pushing i_mtime with a SETATTR operation would look like a cheating userspace. What do you think?

Quoted 40 lines · Maxim V. Patlasov

We should probably look at what NFS is doing.

In case of NFS, the flush does updates the modification time on server. And on client, getattr triggers flush:

     int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat
     *stat)
     {
         ...

         /* Flush out writes to the server in order to update c/mtime.  */
         if (S_ISREG(inode->i_mode)) {
             nfs_inode_dio_wait(inode);
             err = filemap_write_and_wait(inode->i_mapping);
             if (err)
                 goto out;
         }

In another email of this thread you suggested some approach where in-kernel fuse flushes i_mtime to userspace:

So basically what we need is a per-inode flag that says that i_mtime has been updated (it is more recent then what userspace has) and we must update i_mtime *only* in write and not other operations which still do the mtime update in the userspace filesystem. Any operation that modifies i_mtime (and hence invalidate the attributes) must clear the flag. Any other operation which updates or invalidates the attributes must first flush the i_mtime to userspace if the flag is set.

In addition the userspace fileystem need to implement the policy similar to NFS, in which it only updates mtime if it is greater than the current one. This means that we must differentiate between an mtime update due to a buffered write from an mtime update due to an utime (and friends) system call.

Quoted 3 lines

My question is why do we need all these complications if we could follow NFS
way: trigger flush and wait for its (and fuse write-back) completion before sending FUSE_GETATTR to userspace?

Yes, the NFS way seems like a good approach assuming that getattrs are not too frequent. But I guess the fact that NFS does this is a pretty good assurance that will work fine.

Quoted 9 lines

Another concern is about the idea of sending i_mtime to userspace per se. You wrote:

If we are doing buffered writes, then the kernel must update i_mtime on write(2) and must flush that to the userspace filesystem at some point (with a SETTATTR operation).

Quoted 6 lines

Fuse userspace may have its own non-trivial concept of 'modification time'. It's not obliged to advance its mtime on every write. The only requirement is to be consistent: if we expose new data handling READs, mtime must be advanced properly as well. But, for example, the granularity of changes is up to implementation. From this view, in-kenel fuse pushing i_mtime with a SETATTR operation would look like a cheating userspace. What do you think?

I think you are right in that mixing kernel mtime updates with userspace mtime updates doesn't work. Either the kernel should be wholly responsible (which works only for "local" filesystems) or the userspace is fully responsible for mtime updates (which works in all cases but may be suboptimal).

Make balance_dirty_pages start the throttling when the WRITEBACK_TEMP counter is hight ehough. This prevents us from having too many dirty pages on fuse, thus giving the userspace part of it a chance to write stuff properly.

Note, that the existing balance logic is per-bdi, i.e. if the fuse user task gets stuck in the function this means, that it either writes to the mountpoint it serves (but it can deadlock even without the writeback) or it is wrting to some _other_ dirty bdi and in the latter case someone else will free the memory for it.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 mm/page-writeback.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

mm/page-writeback.c +0 -0

index 93d8d2f..7cb54db 100644

Quoted 10 lines · Pavel Emelyanov

Make balance_dirty_pages start the throttling when the WRITEBACK_TEMP counter is hight ehough. This prevents us from having too many dirty pages on fuse, thus giving the userspace part of it a chance to write stuff properly.

Note, that the existing balance logic is per-bdi, i.e. if the fuse user task gets stuck in the function this means, that it either writes to the mountpoint it serves (but it can deadlock even without the writeback) or it is wrting to some _other_ dirty bdi and in the latter case someone else will free the memory for it.

This is not just about deadlocking. Unprivileged fuse filesystems should not impact the operation of other filesystems. I.e. a fuse filesystem which is not making progress writing out pages shouln't cause a write on an unrelated filesystem to block.

I believe this patch breaks that promise.

Thanks,
Miklos

Quoted 18 lines · Miklos Szeredi

Quoted 10 lines · Pavel Emelyanov

Make balance_dirty_pages start the throttling when the WRITEBACK_TEMP counter is hight ehough. This prevents us from having too many dirty pages on fuse, thus giving the userspace part of it a chance to write stuff properly.

Note, that the existing balance logic is per-bdi, i.e. if the fuse user task gets stuck in the function this means, that it either writes to the mountpoint it serves (but it can deadlock even without the writeback) or it is wrting to some _other_ dirty bdi and in the latter case someone else will free the memory for it.

This is not just about deadlocking. Unprivileged fuse filesystems should not impact the operation of other filesystems. I.e. a fuse filesystem which is not making progress writing out pages shouln't cause a write on an unrelated filesystem to block.

Quoted 1 line

I believe this patch breaks that promise.

Hm... I believe it does not, and that's why.

When a task writes to some bdi the balance_dirty_pages will evaluate the amount of time to block this task on based on this bdi dirty set counters. The global stats are only used to a) check whether this decision should be made at all

and b) evaluate the dirty "fraction" of a bdi. That said, even if we stop the fuse daemon (I actually did this) other filesystems won't lock. The global counter would be high, yes, but the dirty set fraction of non-fuse bdi would be low thus allowing others to progress.

Pavel Emelyanov <[email protected]> writes:

Quoted 23 lines · Miklos Szeredi

Quoted 13 lines

Quoted 10 lines · Pavel Emelyanov

Make balance_dirty_pages start the throttling when the WRITEBACK_TEMP counter is hight ehough. This prevents us from having too many dirty pages on fuse, thus giving the userspace part of it a chance to write stuff properly.

Note, that the existing balance logic is per-bdi, i.e. if the fuse user task gets stuck in the function this means, that it either writes to the mountpoint it serves (but it can deadlock even without the writeback) or it is wrting to some _other_ dirty bdi and in the latter case someone else will free the memory for it.

Quoted 6 lines

This is not just about deadlocking. Unprivileged fuse filesystems should not impact the operation of other filesystems. I.e. a fuse filesystem which is not making progress writing out pages shouln't cause a write on an unrelated filesystem to block.

I believe this patch breaks that promise.

Hm... I believe it does not, and that's why.

Quoted 4 lines

When a task writes to some bdi the balance_dirty_pages will evaluate the amount of time to block this task on based on this bdi dirty set counters. The global stats are only used to a) check whether this decision should be made at all

Okay, maybe I'm blind but if this is true, then how is balance_dirty_pages() supposed to ensure that the per-bdi limit is not exceeded?

Quoted 4 lines

and b) evaluate the dirty "fraction" of a bdi. That said, even if we stop the fuse daemon (I actually did this) other filesystems won't lock. The global counter would be high, yes, but the dirty set fraction of non-fuse bdi would be low thus allowing others to progress.

That makes some sense, but it looks to me that FUSE, NFS and friends want a stricter dirty balancing logic that looks at the bdi thresholds even if the global limits are not exceeded.

Thanks,
Miklos
.

Quoted 4 lines · Miklos Szeredi

Okay, maybe I'm blind but if this is true, then how is balance_dirty_pages() supposed to ensure that the per-bdi limit is not exceeded?

The balance_dirty_pages logic is _very_ roughly the the following:

Let this_bdi be a bdi the current task is writing to
Let D be the total amount of dirty and writeback memory (and writeback_tmp after this patch) Let L be the limit of dirty memory (L = ram_size * ratio)
Let d be the amount of dirty and writeback on this_bdi
And let l be the limit of dirty memory on this_bdi

With that the balancer logic look like

while (1) {
	if (D < L)
		return;

	start_background_writeback(this_bdi);

	if (d < l)
		return;

	timeout = get_sleep_timeout(d, l, D, L);
	shcedule_timeout(timeout);
}

The d and l are calculated out of the D and L using this_bdi and global IO completions proportions (with more complexity, but still).

Thus, since we throttle tasks looking ad d and l only we cannot affect all the bdis in the system by live-locking a single one of them.

Accounting for writeback_tmp is required since the D should become high when there are lots of pages in-flight in FUSE. Otherwise, the balance_dirty_pages will not limit the task writing on a fuse mount.