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

[Kevin Wolf]

Am 09.05.2016 um 08:38 hat Raghavendra Talur geschrieben:
> 
> 
> On Mon, Apr 11, 2016 at 9:56 AM, Raghavendra Gowdappa <address@hidden>
> wrote:
> 
> 
>     > +Raghavendra G who implemented this option in write-behind, to this
>     > upstream patch review discussion
> 
>     Thanks Pranith. Kritika did inform us about the discussion. We were 
> working
>     on ways to find out solutions to the problems raised (and it was a long
>     festive weekend in Bangalore).
> 
>     Sorry for top-posting. I am trying to address two issues raised in this
>     mail thread:
>     1. No ways to identify whether setting of option succeeded in gfapi.
>     2. Why retaining cache after fsync failure is not default behavior?
> 
>     1. No ways to identify whether setting of option succeeded in gfapi:
> 
>     I've added Poornima and Raghavendra Talur who work on gfapi to assist on
>     this.
> 
> 
>  There is currently no such feature in gfapi. We could think of two possible
> solutions:
> 
>  a. Have the qemu-glusterfs driver require a version of glusterfs-api.rpm 
> which
> surely has this write behind option. In that case, if you use
> glfs_set_xlator_option before glfs_init with right key and value, there is no
> way the volume set gets rejected. Note that this is a installation time
> dependency, we don't have libgfapi library versioning. This solution is good
> enough, if the logic in qemu is 
> 
> ret = glfs_set_xlator_option;
> if (ret) {
>     exit because we don't have required environment.
> } 
> proceed with work;

This is not good enough. Basically the requirement is: Given a qemu
binary that is put on a machine with a random gluster version installed,
this qemu binary never corrupts a gluster image. Either it errors out or
it works correctly.

Just assuming that the right gluster version is installed is much too
weak, and even version number checks aren't very good when you consider
things like backports.

> b. Second solution is to implement a case in write_behind getxattr FOP to
> handle such request and reply back with the current setting. Look at
> dht_get_real_filename for similar feature. You will have to implement logic
> similar to something like below
> 
> ret = glfs_getxattr ( fs, "/", glusterfs.write-behind-
> resync-failed-syncs-after-fsync-status, &value, size);
> 
> if ( (ret = -1 && errno == ENODATA) || value == 0 ) {
>        // write behind in the stack does not understand this option
>        //  OR
>       //  we have write-behind with required option set to off
>     <work with the assumptions that there are not retries>
> } else {
>     // We do have the required option
> }

Yes, please this one.

Or a new glfs_setxattr_fail() that returns an error if the option didn't
take effect. But I guess a glfs_getxattr() function makes sense anyway.
In qemu, we try to allow querying everything that you can set.

> One negative aspect of both the solutions above is the loosely coupled nature
> of logic. If the behavior ever changes in lower layer(which is gfapi or
> write-behind in this case) the upper layer(qemu) will have to
> a. have a dependency of the sort which defines both the minimum version and
> maximum version of package required
> b. interpret the return values with more if-else conditions to maintain
> backward compatibility.

It's gluster's job to keep the compatibility with existing users of the
APIs. Just like qemu has to make sure that old QMP clients keep working
when we make changes or extensions to the protocol.

In other words: If the behaviour of a lower layer changes while the
upper layer only uses old APIs, that's clearly a bug in the lower layer.

Kevin