[PATCH] Don't stop when syncing a directory returns an error.

[Sergiu Ivanov]

* netfs.c (netfs_attempt_sync): Move err inside the loop.  Declare
final_err.  On an error, store the error and go to the next item.
Check for the directory port being NULL.
---

Hello,

On Fri, Aug 14, 2009 at 05:18:59PM +0200, Thomas Schwinge wrote:
> On Fri, Aug 14, 2009 at 04:36:45PM +0300, Sergiu Ivanov wrote:
> > 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.

I just didn't think about sending it in one message.  (I hope this one
is formatted properly).

> | +  /* 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?

I don't really need to handle any locking here.  The ulfs_chain needs
to be locked only when it is to be modified, while np is already
locked by the caller.

> Also, do you really have to define ulfs -- both ulfs_iterate and
> node_ulfs_iterate_unlocked do that already, isn't it?

The problem is that at each iteration of the syncing loop I need to
(1) know whether the current filesystem is writable and (2) have the
port to its root directory.  The information about filesystems is
stored in ulfs_chain (1), while ports are stored in lists of node_ulfs
associated with every node (2).  ulfs_iterate* define ulfs, which
iterates over the ulfs_chain, while node_ulfs_iterate* define
node_ulfs, which traverses the list of node_ulfs associated with the
given node.  I hope it's clear from my explanation that whichever
macro I choose, I'll still need to keep a separate variable to go
through the other list.

> | +  {
> | +    /* 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.

Note that netfs_attempt_sync is meant to work okay on *every* node
handled by unionfs.  This means that the situation when one of the
entries in the list of ports is MACH_PORT_NULL is completely normal
and valid (consider the situation when a certain directory is only
present in one of the merged filesystems).  I'm strongly inclined to
think that returning an error in a normal situation is not we would
like to have.
 
> | +   && (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).

Sorry, I didn't really notice this detail :-(

Regards,
scolobb

---
 netfs.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/netfs.c b/netfs.c
index b3174fb..2ca8505 100644
--- a/netfs.c
+++ b/netfs.c
@@ -283,7 +283,8 @@ error_t
 netfs_attempt_sync (struct iouser *cred, struct node *np,
                    int wait)
 {
-  error_t err = 0;
+  /* The error we are going to report back (last failure wins).  */
+  error_t final_err = 0;
 
   /* The index of the currently analyzed filesystem.  */
   int i = 0;
@@ -300,23 +301,29 @@ netfs_attempt_sync (struct iouser *cred, struct node *np,
      place, because now iterating over both lists looks ugly.  */
   node_ulfs_iterate_unlocked (np)
   {
+    error_t err;
+
     /* Get the information about the current filesystem.  */
     err = ulfs_get_num (i, &ulfs);
     if (err)
-      break;
+      {
+       final_err = err;
+       continue;
+      }
 
-    if (ulfs->flags & FLAG_ULFS_WRITABLE)
+    if ((node_ulfs->port != MACH_PORT_NULL)
+       && (ulfs->flags & FLAG_ULFS_WRITABLE))
       {
        err = file_sync (node_ulfs->port, wait, 0);
        if (err)
-         break;
+         final_err = err;
       }
 
     ++i;
   }
 
   mutex_unlock (&ulfs_lock);
-  return err;
+  return final_err;
 }
 
 /* This should sync the entire remote filesystem.  If WAIT is set,
-- 
1.6.3.3