Re: [PATCH] Implement the sync libnetfs stubs.

[olafBuddenhagen]

Hi,

On Mon, Aug 17, 2009 at 11:44:59PM +0300, Sergiu Ivanov wrote:

> I think that file_syncfs is equivalent to fsys_syncfs, the difference
> being in the target of invocation (file_syncfs is invoked on a port to
> a file, while fsys_syncfs is invoked on the control port).

Yeah, that's my understanding as well.

> It looks as though the existence of both RPCs were indeed motivated by
> the necessity of solving the problem Fredrik pointed out (syncing
> filesystems of which you are not an owner).

Probably.

> diff --git a/netfs.c b/netfs.c
> index 89d1bf6..0180b1a 100644
> --- a/netfs.c
> +++ b/netfs.c
> @@ -1,5 +1,6 @@
>  /* Hurd unionfs
> -   Copyright (C) 2001, 2002, 2003, 2005 Free Software Foundation, Inc.
> +   Copyright (C) 2001, 2002, 2003, 2005, 2009 Free Software Foundation, Inc.
> +
>     Written by Moritz Schulte <moritz@duesseldorf.ccc.de>.
>  
>     This program is free software; you can redistribute it and/or
> @@ -282,7 +283,45 @@ error_t
>  netfs_attempt_sync (struct iouser *cred, struct node *np,
>                   int wait)
>  {
> -  return EOPNOTSUPP;
> +  /* 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;

I think the initialization of "i" should be as close to the loop as
possible -- after all, it's a loop counter...

> +
> +  /* 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)
> +  {
> +    error_t err;
> +
> +    /* Get the information about the current filesystem.  */
> +    err = ulfs_get_num (i, &ulfs);
> +    assert (err == 0);

Minor nitpick: it's more common to do such checks with "!err".

> +
> +    /* Since `np` may not necessarily be present in every underlying
> +       directory, having a null port is perfectly valid.  */
> +    if ((node_ulfs->port != MACH_PORT_NULL)
> +     && (ulfs->flags & FLAG_ULFS_WRITABLE))

Not sure whether I asked this before: is there actually any reason not
to attempt syncing filesystems without FLAG_ULFS_WRITABLE as well?...

(I don't know how file_sync() or file_syncfs() bahave on filesystems or
nodes that really are not writable -- but IIRC that's not what
FLAG_ULFS_WRITABLE conveys anyways?...)

> +      {
> +     err = file_sync (node_ulfs->port, wait, 0);
> +     if (err)
> +       final_err = err;
> +      }
> +
> +    ++i;
> +  }
> +
> +  mutex_unlock (&ulfs_lock);
> +  return final_err;
>  }
>  
>  /* This should sync the entire remote filesystem.  If WAIT is set,
> @@ -290,7 +329,44 @@ netfs_attempt_sync (struct iouser *cred, struct node *np,
>  error_t
>  netfs_attempt_syncfs (struct iouser *cred, int wait)
>  {
> -  return 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;
> +
> +  /* The information about the currently analyzed filesystem.  */
> +  ulfs_t * ulfs;
> +
> +  mutex_lock (&ulfs_lock);
> +
> +  /* Sync every writable filesystem maintained by unionfs.
> +
> +     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 (netfs_root_node)
> +  {
> +    error_t err;
> +
> +    /* Get the information about the current filesystem.  */
> +    err = ulfs_get_num (i, &ulfs);
> +    assert (err == 0);
> +
> +    /* Note that, unlike the situation in netfs_attempt_sync, having a
> +       null port here is abnormal.  */

Perhaps it would be helpful to state more explicitely that having a NULL
port *on the unionfs root node* is abnormal -- I didn't realize this
point at first.

(Maybe you should actually assert() this.)

> +    if (ulfs->flags & FLAG_ULFS_WRITABLE)
> +      {
> +     err = file_syncfs (node_ulfs->port, wait, 0);
> +     if (err)
> +       final_err = err;
> +      }
> +
> +    ++i;
> +  }
> +
> +  mutex_unlock (&ulfs_lock);
> +  return final_err;
>  }
>  
>  /* lookup */
> -- 
> 1.6.3.3

-antrik-