lkml.org 
[lkml]   [2013]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/4] fuse: writepages: crop secondary requests
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);
>
>



\
 
 \ /
  Last update: 2013-10-03 18:21    [W:0.223 / U:4.960 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site