On 10/03/2013 07:14 PM, Miklos Szeredi wrote:
On Thu, Oct 03, 2013 at 05:28:30PM +0400, Maxim Patlasov wrote:
1. There is an in-flight primary request with a chain of secondary ones.
2. User calls ftruncate(2) to extend file; fuse_set_nowrite() makes
fi->writectr negative and starts waiting for completion of that
in-flight request
3. Userspace fuse daemon ACKs the request and fuse_writepage_end()
is called; it calls __fuse_flush_writepages(), but the latter does
nothing because fi->writectr < 0
4. fuse_do_setattr() proceeds extending i_size and calling
__fuse_release_nowrite(). But now new (increased) i_size will be
used as 'crop' arg of __fuse_flush_writepages()
stale data can leak to the server.
So, lets do this then: skip fuse_flush_writepages() and call
fuse_send_writepage() directly. It will ignore the NOWRITE logic, but that's
okay, this happens rarely and cannot happen more than once in a row.
Does this look good?
Yes, but let's at least add a comment explaining why it's safe. There
are three different cases and what you write above explains only one of
them:
1st case (trivial): there are no concurrent activities using
fuse_set/release_nowrite. Then we're on safe side because
fuse_flush_writepages() would call fuse_send_writepage() anyway.
2nd case: someone called fuse_set_nowrite and it is waiting now for
completion of all in-flight requests. Here what you wrote about
"happening rarely and no more than once" is applicable.
3rd case: someone (e.g. fuse_do_setattr()) is in the middle of
fuse_set_nowrite..fuse_release_nowrite section. The fact that
fuse_set_nowrite returned implies that all in-flight requests were
completed along with all its secondary requests (because we increment
writectr for a secondry before decrementing it for the primary -- that's
how fuse_writepage_end is implemeted). Further requests are blocked by
negative writectr. Hence there cannot be any in-flight requests and no
invocations of fuse_writepage_end while we're in
fuse_set_nowrite..fuse_release_nowrite section.
It looks obvious now, but I'm not sure we'll able to recollect it later.
Can you actually trigger this path with your testing?
No.
Thanks,
Maxim
Thanks,
Miklos
Index: linux/fs/fuse/file.c
===================================================================
--- linux.orig/fs/fuse/file.c 2013-10-03 12:12:33.480918954 +0200
+++ linux/fs/fuse/file.c 2013-10-03 17:06:23.702510854 +0200
@@ -1436,12 +1436,12 @@ static void fuse_writepage_finish(struct
}
/* Called under fc->lock, may release and reacquire it */
-static void fuse_send_writepage(struct fuse_conn *fc, struct fuse_req *req)
+static void fuse_send_writepage(struct fuse_conn *fc, struct fuse_req *req,
+ loff_t size)
__releases(fc->lock)
__acquires(fc->lock)
{
struct fuse_inode *fi = get_fuse_inode(req->inode);
- loff_t size = i_size_read(req->inode);
struct fuse_write_in *inarg = &req->misc.write.in;
__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
@@ -1482,12 +1482,13 @@ __acquires(fc->lock)
{
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
+ size_t crop = i_size_read(inode);
struct fuse_req *req;
while (fi->writectr >= 0 && !list_empty(&fi->queued_writes)) {
req = list_entry(fi->queued_writes.next, struct fuse_req, list);
list_del_init(&req->list);
- fuse_send_writepage(fc, req);
+ fuse_send_writepage(fc, req, crop);
}
}
@@ -1499,12 +1500,13 @@ static void fuse_writepage_end(struct fu
mapping_set_error(inode->i_mapping, req->out.h.error);
spin_lock(&fc->lock);
while (req->misc.write.next) {
+ struct fuse_conn *fc = get_fuse_conn(inode);
+ struct fuse_write_in *inarg = &req->misc.write.in;
struct fuse_req *next = req->misc.write.next;
req->misc.write.next = next->misc.write.next;
next->misc.write.next = NULL;
list_add(&next->writepages_entry, &fi->writepages);
- list_add_tail(&next->list, &fi->queued_writes);
- fuse_flush_writepages(inode);
+ fuse_send_writepage(fc, next, inarg->offset + inarg->size);
}
fi->writectr--;
fuse_writepage_finish(fc, req);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
