Re: [Qemu-devel] [Qemu-block] [PATCH for-2.6 2/2] block/gluster: prevent data loss after i/o error

[Pranith Kumar Karampuri <pkarampu@redhat.com>]

+Raghavendra G who implemented this option in write-behind, to this 
upstream patch review discussion

Pranith
On 04/06/2016 06:50 PM, Kevin Wolf wrote:
> Am 06.04.2016 um 15:10 hat Jeff Cody geschrieben:
>> On Wed, Apr 06, 2016 at 01:51:59PM +0200, Kevin Wolf wrote:
>>> Am 06.04.2016 um 13:41 hat Kevin Wolf geschrieben:
>>>> Am 06.04.2016 um 13:19 hat Ric Wheeler geschrieben:
>>>>> We had a thread discussing this not on the upstream list.
>>>>>
>>>>> My summary of the thread is that I don't understand why gluster
>>>>> should drop cached data after a failed fsync() for any open file.
>>>> It certainly shouldn't, but it does by default. :-)
>>>>
>>>> Have a look at commit 3fcead2d in glusterfs.git, which at least
>>>> introduces an option to get usable behaviour:
>>>>
>>>>      { .key = {"resync-failed-syncs-after-fsync"},
>>>>        .type = GF_OPTION_TYPE_BOOL,
>>>>        .default_value = "off",
>>>>        .description = "If sync of \"cached-writes issued before fsync\" "
>>>>                       "(to backend) fails, this option configures whether "
>>>>                       "to retry syncing them after fsync or forget them. "
>>>>                       "If set to on, cached-writes are retried "
>>>>                       "till a \"flush\" fop (or a successful sync) on sync "
>>>>                       "failures. "
>>>>                       "fsync itself is failed irrespective of the value of "
>>>>                       "this option. ",
>>>>      },
>>>>
>>>> As you can see, the default is still to drop cached data, and this is
>>>> with the file still opened. qemu needs to make sure that this option is
>>>> set, and if Jeff's comment in the code below is right, there is no way
>>>> currently to make sure that the option isn't silently ignored.
>>>>
>>>> Can we get some function that sets an option and fails if the option is
>>>> unknown? Or one that queries the state after setting an option, so we
>>>> can check whether we succeeded in switching to the mode we need?
>>>>
>>>>> For closed files, I think it might still happen but this is the same
>>>>> as any file system (and unlikely to be the case for qemu?).
>>>> Our problem is only with open images. Dropping caches for files that
>>>> qemu doesn't use any more is fine as far as I'm concerned.
>>>>
>>>> Note that our usage can involve cases where we reopen a file with
>>>> different flags, i.e. first open a second file descriptor, then close
>>>> the first one. The image was never completely closed here and we would
>>>> still want the cache to preserve our data in such cases.
>>> Hm, actually, maybe we should just call bdrv_flush() before reopening an
>>> image, and if an error is returned, we abort the reopen. It's far from
>>> being a hot path, so the overhead of a flush shouldn't matter, and it
>>> seems we're taking an unnecessary risk without doing this.
>>>
>> [I seemed to have been dropped from the cc]
>>
>> Are you talking about doing a bdrv_flush() on the new descriptor (i.e.
>> reop_s->glfs)?  Because otherwise, we already do this in
>> bdrv_reopen_prepare() on the original fd.  It happens right before the call
>> to drv->bdrv_reopen_prepare():
>>
>>
>> 2020     ret = bdrv_flush(reopen_state->bs);
>> 2021     if (ret) {
>> 2022         error_setg_errno(errp, -ret, "Error flushing drive");
>> 2023         goto error;
>> 2024     }
>> 2025
>> 2026     if (drv->bdrv_reopen_prepare) {
>> 2027         ret = drv->bdrv_reopen_prepare(reopen_state, queue, &local_err);
> Ah, thanks. Yes, this is what I meant. I expected it somewhere close to
> the bdrv_drain_all() call, so I missed the call you quoted. So that's
> good news, at least this part of the problem doesn't exist then. :-)
>
> Kevin
>
>>>>> I will note that Linux in general had (still has I think?) the
>>>>> behavior that once the process closes a file (or exits), we lose
>>>>> context to return an error to. From that point on, any failed IO
>>>>> from the page cache to the target disk will be dropped from cache.
>>>>> To hold things in the cache would lead it to fill with old data that
>>>>> is not really recoverable and we have no good way to know that the
>>>>> situation is repairable and how long that might take. Upstream
>>>>> kernel people have debated this, the behavior might be tweaked for
>>>>> certain types of errors.
>>>> That's fine, we just don't want the next fsync() to signal success when
>>>> in reality the cache has thrown away our data. As soon as we close the
>>>> image, there is no next fsync(), so you can do whatever you like.
>>>>
>>>> Kevin
>>>>
>>>>> On 04/06/2016 07:02 AM, Kevin Wolf wrote:
>>>>>> [ Adding some CCs ]
>>>>>>
>>>>>> Am 06.04.2016 um 05:29 hat Jeff Cody geschrieben:
>>>>>>> Upon receiving an I/O error after an fsync, by default gluster will
>>>>>>> dump its cache.  However, QEMU will retry the fsync, which is especially
>>>>>>> useful when encountering errors such as ENOSPC when using the werror=stop
>>>>>>> option.  When using caching with gluster, however, the last written data
>>>>>>> will be lost upon encountering ENOSPC.  Using the cache xlator option of
>>>>>>> 'resync-failed-syncs-after-fsync' should cause gluster to retain the
>>>>>>> cached data after a failed fsync, so that ENOSPC and other transient
>>>>>>> errors are recoverable.
>>>>>>>
>>>>>>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>>>>>> ---
>>>>>>>   block/gluster.c | 27 +++++++++++++++++++++++++++
>>>>>>>   configure       |  8 ++++++++
>>>>>>>   2 files changed, 35 insertions(+)
>>>>>>>
>>>>>>> diff --git a/block/gluster.c b/block/gluster.c
>>>>>>> index 30a827e..b1cf71b 100644
>>>>>>> --- a/block/gluster.c
>>>>>>> +++ b/block/gluster.c
>>>>>>> @@ -330,6 +330,23 @@ static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
>>>>>>>           goto out;
>>>>>>>       }
>>>>>>> +#ifdef CONFIG_GLUSTERFS_XLATOR_OPT
>>>>>>> +    /* Without this, if fsync fails for a recoverable reason (for instance,
>>>>>>> +     * ENOSPC), gluster will dump its cache, preventing retries.  This means
>>>>>>> +     * almost certain data loss.  Not all gluster versions support the
>>>>>>> +     * 'resync-failed-syncs-after-fsync' key value, but there is no way to
>>>>>>> +     * discover during runtime if it is supported (this api returns success for
>>>>>>> +     * unknown key/value pairs) */
>>>>>> Honestly, this sucks. There is apparently no way to operate gluster so
>>>>>> we can safely recover after a failed fsync. "We hope everything is fine,
>>>>>> but depending on your gluster version, we may now corrupt your image"
>>>>>> isn't very good.
>>>>>>
>>>>>> We need to consider very carefully if this is good enough to go on after
>>>>>> an error. I'm currently leaning towards "no". That is, we should only
>>>>>> enable this after Gluster provides us a way to make sure that the option
>>>>>> is really set.
>>>>>>
>>>>>>> +    ret = glfs_set_xlator_option (s->glfs, "*-write-behind",
>>>>>>> +                                           "resync-failed-syncs-after-fsync",
>>>>>>> +                                           "on");
>>>>>>> +    if (ret < 0) {
>>>>>>> +        error_setg_errno(errp, errno, "Unable to set xlator key/value pair");
>>>>>>> +        ret = -errno;
>>>>>>> +        goto out;
>>>>>>> +    }
>>>>>>> +#endif
>>>>>> We also need to consider the case without CONFIG_GLUSTERFS_XLATOR_OPT.
>>>>>> In this case (as well as theoretically in the case that the option
>>>>>> didn't take effect - if only we could know about it), a failed
>>>>>> glfs_fsync_async() is fatal and we need to stop operating on the image,
>>>>>> i.e. set bs->drv = NULL like when we detect corruption in qcow2 images.
>>>>>> The guest will see a broken disk that fails all I/O requests, but that's
>>>>>> better than corrupting data.
>>>>>>
>>>>>> Kevin