Re: [PATCH 2/4] fuse: writepages: crop secondary requests

[Maxim Patlasov <mpatlasov@parallels.com>]

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);
>
>