Re: [PATCH] Implement the sync libnetfs stubs.

[Thomas Schwinge]

Hello!

On Fri, Aug 14, 2009 at 04:36:45PM +0300, Sergiu Ivanov wrote:
> 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 :-(

Heh, no problem: that's simply the commit-hurd mailing list where all
commits (should) show up.


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

Why did you send it in a separate message by the way?  Anyways, I'll post
my few comments in here.

| --- a/netfs.c
| +++ b/netfs.c
| @@ -282,7 +283,45 @@ error_t
|  netfs_attempt_sync (struct iouser *cred, struct node *np,
|                   int wait)
|  {
| -  return EOPNOTSUPP;

Your new patch should be based on what is in unionfs' master branch at
the moment, that is, it will be committed on top of the previously
committed patch.

| +  error_t err = 0;

Generally, move local variables into the innermost frame where they are
needed.  Here, move err into the node_ulfs_iterate_unlocked loop.

| +  /* A pessimistic combination (failure wins) of the results of all
| +     calls to file_sync.  */
| +  error_t combined_err = 0;

``combination of'' sounds like ``combined_err |= ...'' (which is wrong,
of course).  Perhaps use the name final_err, and adjust the comment
accordingly, perhaps something like: ``The error we're finally going to
report back: the last failure wins.''

| +  /* The index of the currently analyzed filesystem.  */
| +  int i = 0;
| +
| +  /* The information about the currently analyzed filesystem.  */
| +  ulfs_t * ulfs;
| +
| +  mutex_lock (&ulfs_lock);
| +
| +  /* Sync every writable directory associated with `np`.
| +
| +     TODO: Rewrite this after having modified ulfs.c and node.c to
| +     store the paths and ports to the underlying directories in one
| +     place, because now iterating over both lists looks ugly.  */
| +  node_ulfs_iterate_unlocked (np)

Having had a look at ulfs.h again: why didn't you use ulfs_iterate and
have that one handle the locking?  Also, do you really have to define
ulfs -- both ulfs_iterate and node_ulfs_iterate_unlocked do that already,
isn't it?

| +  {
| +    /* Get the information about the current filesystem.  */
| +    err = ulfs_get_num (i, &ulfs);
| +    if (err)
| +      combined_err = err;
| +
| +    if ((node_ulfs->port != MACH_PORT_NULL)

What is the rationale for this check -- do we really need it, or won't
calling file_sync ([invalid port], ...) already do the right thing
(nothing) and report back a suitable error value?  Perhaps I'm missing
something.

| +     && (ulfs->flags & FLAG_ULFS_WRITABLE))

If you got err != 0 you probably shouldn't proceed here to dereference
ulfs, and instead continue with the next ulfs (as I had it in my
suggestion).

| +      {
| +     err = file_sync (node_ulfs->port, wait, 0);
| +     if (err)
| +       combined_err = err;
| +      }
| +
| +    ++i;
| +  }
| +
| +  mutex_unlock (&ulfs_lock);
| +  return combined_err;


Regards,
 Thomas

signature.asc
Description: Digital signature