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

[Maxim Patlasov <mpatlasov@parallels.com>]

On 10/09/2013 12:20 PM, Maxim Patlasov wrote:
> Hi Miklos,
>
> On 10/03/2013 08:22 PM, Maxim Patlasov wrote:
>> On 10/03/2013 08:09 PM, Miklos Szeredi wrote:
>>> On Thu, Oct 3, 2013 at 5:50 PM, Maxim Patlasov <mpatlasov@parallels.com> wrote:
>>>> 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.
>>> Added your analysis as a comment and all patches pushed to writepages.v2.
>> Great! So I can proceed with re-basing the rest of
>> writeback-cache-policy pile to writepages.v2 soon.
> More testing (with writeback-cache-policy enabled) revealed another bug
> in that implementation. The problem deals with a write(2) extending i_size:
>
> 1. There is an in-flight primary request now. It was properly cropped
> against i_size which was valid then and is valid now. So there is a page
> in the request that will be written to the server partially.
> 2. write(2) to a distant offset makes a hole and extends i_size.
> 3. write(2) populates that whole page by new user data.
> 4. Writeback happens and fuse_writepage_in_flight() attaches a secondary
> request to the primary request.
> 5. fuse_writepage_end() for the primary request calls
> fuse_send_writepage() with 'crop' arg equal to "inarg->offset +
> inarg->size". But inarg->size was calculated before i_size extension, so
> the second request will be cropped as well as primary. The result is
> that the tail of secondary request populated by valid actual user data
> won't be stored on the server.
>
> The problem will be hidden by adding fuse_wait_on_page_writeback() to
> write_begin fuse method, but the implementation will remain unsafe if we
> believe a re-dirty may happen spontaneously. Straightforward solution
> would be to crop secondary requests at the time of their queuing (using
> actual i_size). Then fuse_send_writepage() would crop further only if
> i_size shrunk. Please let me know if you come up with a smarter idea.

Sorry for flooding. I've just realized that the problem is actually 
solved (not "hidden") by adding fuse_wait_on_page_writeback() to 
write_begin fuse method. No need to rework cropping mechanism again.

Thanks,
Maxim