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

[Raghavendra Talur]

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;

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

}

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.

 We are thinking of other ways too, but given above are current solutions that come to mind.

Thanks,

Raghavendra Talur

2. Why retaining cache after fsync failure is not default behavior?

It was mostly to not to break two synchronized applications, which rely on fsync failures to retry. Details of discussion can be found below. The other reason was we could not figure out what POSIX's take on the state of earlier write after fsync failure (Other filesystems xfs, ext4 had non-uniform behavior). The question more specifically was "is it correct for a write issued before a failed fsync to succeed on the backend storage (persisting happened after fsync failure)?". I've also added Vijay Bellur who was involved in the earlier discussion to cc list.

Following is the discussion we had earlier with Kevin, Jeff Cody and others on the same topic. I am quoting it verbatim below.

<quote>

> > > > > I would actually argue that this setting would be right for everyone,
> > > > > not just qemu. Can you think of a case where keeping the data cached
> > > > > after a failed fsync would actively hurt any application? I can only
> > > > > think of cases where data is unnecessarily lost if data is dropped.
> > > > >
> > > >
> > > > I worry about use cases with concurrent writers to the same file and
> > > > how different applications would handle fsync() failures with our new
> > > > behavior.
> > >
> > > Any specific scenario you're worried about?
> > >
> > > > Keeping the known old behavior as the default will ensure that we do
> > > > not break anything once this is out. qemu/virt users with gluster are
> > > > accustomed to setting the virt group and hence no additional knobs
> > > > would need to be altered by them.
> > >
> > > Not changing anything is a safe way to avoid regressions. But it's also
> > > a safe way to leave bugs unfixed. I would say that the current behavíour
> > > is at least borderline buggy and very hard for applications to handle
> > > correctly.
> >
> > I tend to agree with Kevin on this. If we've an error handling strategy
> > that
> > is posix-complaint, I don't think there is need to add one more option.
> > Most
> > of the times options tend to be used in default values, which is equivalent
> > to not providing an option at all. However, before doing that its better we
> > think through whether it can affect any existing deployments adversely
> > (even
> > when they are not posix-complaint).
> >
>
> One pattern that I can think of -
>
> Applications that operate on the same file from different clients through
> some locking or other co-ordination would have this pattern:
>
> lock (file), write (file), fsync (file), unlock (file);
>
> Now if the first fsync() fails, based on application logic the offset used
> for the failed write + fsync could be re-utilized by a co-ordinating
> application on another node to write out legitimate data. When control
> returns back to the application that received a failure, the subsequent
> write + fsync can cause data to be overwritten at the old offset along with
> new data being written at the new offset.

Yeah. I agree. Co-ordinated applications on different mounts will have issues, if they are working on the assumption that after fsync no older writes will hit the backend. Given that there seems to be a fair bit of confusion on status of retry of older writes after an fsync failure, we can expect some regressions. So, to summarize,

1. Keep the behaviour in patchset 11 of [1] as default. Both fsync and flush act as barriers and will make sure either
   a. older writes are synced to backend
   b. old writes are failed and never retried.

   after a failed fsync/flush.

2. Modify the behaviour of patchset 11 of [1] to keep failed syncs after a failed fsync and retry them till a flush. After a flush, no retries of failed syncs will be attempted. This behaviour will be introduced as an option.

3. Transient and non-transient errors will be treated similarly and failed syncs will be retried alike.

Does everyone agree on the above points? If yes, I'll modify [1] accordingly.

[1] http://review.gluster.org/#/c/12594/

</quote>