Re: [PATCH] Implement the sync libnetfs stubs.

[Sergiu Ivanov]

Hello,

On Sat, Jul 18, 2009 at 08:08:20AM +0200, olafBuddenhagen@gmx.net wrote:
> 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:
>
> > > > +  node_ulfs_iterate_unlocked (np)
> > > > +  {
> > > > +    /* Get the information about the current filesystem.  */
> > > > +    err = ulfs_get_num (i, &ulfs);
> > > 
> > > I don't think you really got the idea of the iterator... No need for
> > > "i".
> > 
> > Well :-) I'd say that the idea of an iterator is one of my favourites,
> > so I'm rather inclined to think that I understand it pretty okay ;-)
> > 
> > The problem is that unionfs has a separate list of information about
> > the underlying filesystems and then, each node has a list of *ports*
> > to the underlying filesystems, too.  This means that I need to operate
> > on *two* separate lists to do the syncing: from the first list I get
> > the information about whether the filesystem is writable and from the
> > second one I get the port.  node_ulfs_iterate_unlocked takes me
> > through the list of filesystems in which this node is present and I
> > have to maintain an index into the (parallel) list of underlying
> > filesystems to extract the information about the current filesystem.
> 
> I see. You are right of course -- my comment was too rash; I didn't
> actually look at the details. Sorry.

No problem :-)
 
> So there is no other way to associate the two lists? This is ugly
> indeed. In this case, I think it would be better not to use the iterator
> at all -- what you did here looks really hackish, and it breaks the
> iterator paradigm anyways...

Yeah, true, it breaks the paradigm.  However, I actually borrowed this
piece of code from node_init_root (node.c), so this is the style used
in unionfs.  Should I forget about consistency in this case, what do
you think?
 
> > > > @@ -290,7 +322,10 @@ netfs_attempt_sync (struct iouser *cred, struct 
> > > > node *np,
> > > >  error_t
> > > >  netfs_attempt_syncfs (struct iouser *cred, int wait)
> > > >  {
> > > > -  return 0;
> > > > +  /* The complete list of ports to the merged filesystems is
> > > > +     maintained in the root node of unionfs, so if we sync it, we sync
> > > > +     every single merged directory.  */
> > > > +  return netfs_attempt_sync (cred, netfs_root_node, wait);
> > > 
> > > I'm don't think this is really the right approach. Why not forward the
> > > syncfs to all unioned filesystems?
> > 
> > Fredrik told me in another mail that it might happen that I won't have
> > the right to get the control port of the filesystem I'm working with.
> 
> We don't need the control port -- file_syncfs(), as the name says, is a
> file RPC, not an fsys RPC.

Ah, so you mean forwarding syncfs to all unioned *directories*?
Sorry, I thought your were talking about doing fsys_syncfs on all
unioned *filesystems* :-)

In this case, I'd tell you that the current implementation does
exactly what you are talking about: sends file_sync to all writeable
directories maintained by unionfs :-) You see, the list of *ports* to
the directories is stored in the root node of unionfs (only), so doing
netfs_attempt_sync on netfs_root_node is a pretty natural choice,
IMHO.  The ULFS list (maintained in ulfs.[ch]) is actually only a list
of paths and attributes.

I agree that such a structure is a messy one, but correcting it should
go in a separate large clean-up patch (or a series, maybe).  Even the
author of ulfs.c (the comment says Moritz Schulte) remarks in a
comment: ``FIXME: Ugly as hell. Rewrite the whole ulfs.c''
(ulfs.c:ulfs_check) :-)
 
> > OTOH, I'm not sure whether syncing the whole filesystem is that very
> > good, because in this way we lose the specificity: it's very probable
> > that such syncing will employ a larger number of directories than just
> > those merged by unionfs.
> 
> So? Syncing more than necessary is never a problem... (Except for
> performance perhaps.)
>
> Syncing is safety measure -- it can be too little, but it can never be
> too much :-)

If syncing is a safety measure, it shouldn't be that critical if it
takes a little more time :-)

It occurred to me that syncing more than necessary may trigger some
unwanted effects (like invoking broken implementations of
fsys_syncfs), that is why I mentioned specificity.

Regards,
scolobb