Re: [PATCH] Implement the sync libnetfs stubs.

[olafBuddenhagen]

Hi,

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:

> > > +   Modified by Sergiu Ivanov <unlimitedscolobb@gmail.com>.
> > > +
> > 
> > You can do this if it's important to you -- but so far it's not common
> > in the Hurd to include author information for every change made to a
> > file... In fact, many files do not even have the original author
> > mentioned.
> 
> No, it's not that important to me :-) I just tried to be consistent.
> Do I understand it right that only the copyright years update is
> compulsory?

Yes.

The copyright is with the FSF; the actual author doesn't matter at all
in this case...

> > > +  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.

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...

> > > @@ -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.

> 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 :-)

-antrik-