Re: [PATCH] Implement the sync libnetfs stubs.

[Sergiu Ivanov]

Hello,

On Tue, Aug 11, 2009 at 11:42:41AM +0200, Thomas Schwinge wrote:
> This comment is based on the version of the patch that you installed into
> master.  (By the way: this commit didn't show up on commit-hurd; I'll
> have a look at that.)

Is it my duty to look after my commits showing up on commit-hurd?  If
so, I'm sorry and please tell me how to do that, because I don't even
know what ``commit-hurd'' is :-(
 
> On Sun, Jul 12, 2009 at 10:50:07PM +0300, Sergiu Ivanov wrote:
> > On Sat, Jul 11, 2009 at 03:56:02AM +0200, olafBuddenhagen@gmx.net wrote:
> > > On Wed, Jul 08, 2009 at 10:30:41PM +0300, Sergiu Ivanov wrote:
> > > > +    if (err)
> > > > +      break;
> > > 
> > > I wonder whether it wouldn't perhaps be better to continue in spite of
> > > errors?...
> > 
> > Yes, it's definitely better to do it like that.  And also, I think I
> > forgot to add the check for the validity of the port corresponding to
> > the current filesystem :-(

I had completely forgotten about this :-( My new patch includes the
check, too.

> > > > +
> > > > +    if (ulfs->flags & FLAG_ULFS_WRITABLE)
> > > > +      {
> > > > +       err = file_sync (node_ulfs->port, wait, 0);
> > > > +       if (err)
> > > > +         break;
> > > > +      }
> > > > +
> > > > +    ++i;
> > > > +  }
> 
> I agree that syncing the other nodes should continue even if any of the
> nodes fail to do so.  Or is there a reason not to do it like that?  My
> idea is that all nodes are tought to receive the sync command in
> parallel, and then all their return values are collected and combined in
> a passimistic manner (that is, failure wins).  If you agree, then please
> change this code accordingly.  Like this perhaps (totally untested)?
> 
>     node_ulfs_iterate_unlocked (np)
>     {
>       error_t err_l = ulfs_get_num (i, &ulfs);
>       if (! err_l
>           && (ulfs->flags & FLAG_ULFS_WRITABLE))
>         {
>           err_l = file_sync (node_ulfs->port, wait, 0);
>           /* We can only report back a single error value; the first one
>              wins.  */
>           if (! err)
>             err = err_l;
>         }
>       ++i;
>     }

I agree with your statement about always syncing all nodes and, IIRC,
antrik has already mentioned that, and I have forgotten about that :-(

I'm rather inclined to implement the ``last one wins'' approach, since
it really is simpler and more common.  Please, take a look at my new
patch and tell me if it's acceptable.

Regards,
scolobb