[PATCH] Reference counting for file system locations

[Sebastian Huber]

Hello Gedare,

thanks for the review.  I changed the things like this:

http://git.rtems.org/sebh/rtems.git/commit/?h=fsrefcount&id=a78e2e075bcd28f91c82c99c8ca5b8a1e73c4cc1

On 02/27/2012 10:28 PM, Gedare Bloom wrote:
> On Sat, Feb 18, 2012 at 3:25 AM, Sebastian Huber
> <sebastian.huber at embedded-brains.de>  wrote:
>> On 17/02/12 17:39, Sebastian Huber wrote:
>>>
>>> Hello,
>>>
>>> sorry that this patch is so huge, but it is hard to get it smaller since
>>> the introduction of reference counting was quite a massive undertaking.
>>>
>>> Please review and test this patch.  A fix for the file system in
>>> tfsDriver.c is not included (I will do this after the review).
>>>
>>> Have a nice day!
>>>
>>
>> Somehow the git send-email
>> 0001-Reference-counting-for-file-system-locations.patch didn't work.  The
>> patch can be found here:
>>
>> http://git.rtems.org/sebh/rtems.git/commit/?h=fsrefcount
>>
> This looks like it was quite the undertaking, and it appears to have
> been done well. I have read through the code, but I have not yet had a
> chance to try it out. With any luck I can get a build going soon and
> run the tests. I did not spend much time looking at the testsuites or
> the individual fs code although I did skim them.  Here are my
> comments, some of which may be questions stemming from my own
> not-understanding / curiosity:
>
> +++ b/c/src/lib/libbsp/shared/umon/tfsDriver.c
> +static void rtems_tfs_eval_path(rtems_filesystem_eval_path_context_t *self)
> + {
> +  int eval_flags = rtems_filesystem_eval_path_get_flags(self);
> +
> +  if ((eval_flags&  RTEMS_LIBIO_MAKE) == 0) {
> +    eval_flags&= RTEMS_LIBIO_PERMS_READ | RTEMS_LIBIO_PERMS_WRITE;
> +    if (
> +      eval_flags == 0
> +        || eval_flags == RTEMS_LIBIO_PERMS_READ
> +        || eval_flags == RTEMS_LIBIO_PERMS_WRITE
> Is it an error to have both read and write? That is the only 'else' case,
> so this looks equivalent to if ( eval_flags != RTEMS_LIBIO_PERMS_RDW )
> .. otherwise
> something odd is going on here. Also, why doesn't this function use
> the rtems_filesystem_eval_path_generic code?
>

Both read and write is an error since we don't support seek?  It is the 
existing behaviour.  My intention was not to add more features.  The patch is 
large enough as it is.  I tried to only convert to the new API, nothing more.

>
> @@ -671,68 +593,92 @@ static int rtems_tfs_ioctl(
> +  if (ret != TFS_OKAY)
> +    return(-1);
> +
> +  return(-1);
> +}
> this chunk always returns -1 so something seems wrong here.

Yes, one more bug (the TFS and FTPFS was completely broken before the changes). 
  I fix this one.

>
>
>
> +++ b/cpukit/include/rtems/fs.h
>
> +typedef struct rtems_filesystem_global_location_t {
> +  rtems_filesystem_location_info_t location;
> +  int reference_count;
> +  struct rtems_filesystem_global_location_t *deferred_released_next;
>
> At first glance its not obvious deferred_released_next implements a simple fifo
> maybe just add some more to the comment above about deferred release.
> Why not using an rtems_chain to manage the deferred release nodes?

This saves one pointer and we don't need constant time extract.

>
>
> +++ b/cpukit/libcsupport/include/rtems/libio.h
>
> +typedef struct {
> +  const char *path;
> +  size_t pathlen;
> +  const char *token;
> +  size_t tokenlen;
> +  int flags;
> +  int recursionlevel;
> +  rtems_filesystem_location_info_t currentloc;
> +  rtems_filesystem_global_location_t *rootloc;
> +  rtems_filesystem_global_location_t *startloc;
> +} rtems_filesystem_eval_path_context_t;
>
> Might be nice to know a little more about these fields.

Fixed.

>
>
> + * @brief Changes the mode of a node.
> + *
> + * @param[in, out] iop The IO pointer.
> + * @param[in] mode The new mode of the node
> + *
> + * @retval 0 Successful operation.
> + * @retval -1 An error occured.  The errno is set to indicate the error.
> + *
> + * @see rtems_filesystem_default_fchmod().
>    */
> +typedef int (*rtems_filesystem_fchmod_t)(
> +  const rtems_filesystem_location_info_t *loc,
> +  mode_t mode
>   );
> outdated comment above?
> s/@param[in, out] iop The IO pointer./@param[in] loc The location of the node./

Fixed.

>
>
> + * @brief Unmounts a file system instance in a mount point (directory).
> + *
> + * In case this function is successful the file system instance will be marked
> + * as unmounted.  The file system instance will be destroyed then the last
> s/then/when

Fixed.

>
>
> +/**
> + * @brief File system operations table with default operations.
> + */
> +extern const rtems_filesystem_operations_table
> +rtems_filesystem_operations_default;
> indent nested line?
>
>
> +extern const rtems_filesystem_file_handlers_r
> +rtems_filesystem_handlers_default;
> indent?

Fixed.

>
>
> -#define RTEMS_LIBIO_PERMS_READ   S_IROTH
> -#define RTEMS_LIBIO_PERMS_WRITE  S_IWOTH
> -#define RTEMS_LIBIO_PERMS_RDWR   (S_IROTH|S_IWOTH)
> -#define RTEMS_LIBIO_PERMS_EXEC   S_IXOTH
> +#define RTEMS_LIBIO_PERMS_READ 0x4
> +#define RTEMS_LIBIO_PERMS_WRITE 0x2
> +#define RTEMS_LIBIO_PERMS_EXEC 0x1
> Why changing these away from the S_ versions?
> Are they still compatible with mode_t bits?
> Are these the values for the 'flags' field of eval_path_context_t?

Yes, this is a bit ugly.  I intend to add static asserts to ensure the the S_* 
and the path evaluation flags are compatible.  Maybe we should rename these 
defines in a follow up patch.

>
>
> +typedef struct {
> +  const char *source;
> +  const char *target;
> +  const char *filesystemtype;
> +  rtems_filesystem_options_t options;
> +  const void *data;
> +} rtems_filesystem_mount_configuration;
> what do fields mean?

See mount().

>
>
> +++ b/cpukit/libcsupport/include/rtems/libio_.h
>
> +void rtems_filesystem_clonenode(
> +  rtems_filesystem_location_info_t *clone,
> +  const rtems_filesystem_location_info_t *master
> +);
> What is the difference between filesystem_clonenode and filesystem_clone?

I have now

rtems_filesystem_clonenode_t clonenod_h;
rtems_filesystem_freenode_t freenod_h;

> Maybe rename: filesystem_location_clone().

I wanted to stay in line with rtems_filesystem_freenode().

>
>
> +#define rtems_filesystem_mt_entry_lock_context( ctx ) rtems_interrupt_level ctx
> I found this confusing because it is a declaration and not a function call
> Maybe rtems_filesystem_mt_entry_declare_lock( ctx )

On a SMP configuration this may change from interrupt disable/enable to 
something else.

>
>
> +typedef struct {
> +  rtems_filesystem_eval_path_is_directory is_directory;
> +  rtems_filesystem_eval_path_eval_token eval_token;
> +} rtems_filesystem_eval_path_generic_config;
> I think generic_operations is a more accurate name, or callouts,
> or hooks, or something like that; config seems like it should be some
> data (not functions) that determines what should be done. Maybe it's just me.

Currently we have only handlers, but this may change in the future.

>
>
> +rtems_filesystem_location_info_t *rtems_filesystem_location_initialize(
> +  rtems_filesystem_location_info_t *dst,
> +  const rtems_filesystem_location_info_t *src
> +);
> When I think of initialize I think of initializing to zero.
> I would like to consider renaming this function to be location_copy
> or location_clone. I would leave the location_initialize_to_null name as-is.

Changed it to

rtems_filesystem_location_copy()

>
>
> +static inline void rtems_filesystem_location_initialize_to_null(
> are we supposed to use inline in cpukit? or RTEMS_INLINE_ROUTINE? Same
> with rest of inline functions defined here.

Please grep for 'static inline' in the cpukit.

>
>
> +rtems_filesystem_global_location_t *rtems_filesystem_global_location_transform(
> +  rtems_filesystem_location_info_t *src
> +);
> maybe rename to filesystem_location_transform_to_global() or similar?

I like this name, but most other operations with global locations start with 
rtems_filesystem_global_location_*.

> Could use a couple comments about this function. May block; obtains newly
> created global location. I notice that before this is called the src location
> always is extracted from current_loc--does this imply some pre-requisite about
> the src location?

No, this is a coincidence.

>
>
> +void rtems_filesystem_global_location_assign(
> +  rtems_filesystem_global_location_t **lhs_global_loc_ptr,
> +  rtems_filesystem_global_location_t *rhs_global_loc
> +);
> should rhs be const?

No, we assign it to a non-const pointer.

>
>
> +void rtems_filesystem_replace_with_null_location(
> +  rtems_filesystem_location_info_t *replace
> +);
> +
> +void rtems_filesystem_get_and_replace_with_null_location(
> +  rtems_filesystem_location_info_t *get,
> +  rtems_filesystem_location_info_t *replace
> +);
> Many of the functions are named like rtems_filesystem_global_location_xxx
> and rtems_filesystem_location_xxx. Consider whether to rename these two
> functions to e.g. rtems_filesystem_location_replace_with_null/get_and_replace.
> I also suggest rtems_filesystem_location_detach/copy_and_detach.
> Replacing get with copy seems sensible if location_initialize is replaced
> by location_copy.

Changed it to

rtems_filesystem_location_detach()
rtems_filesystem_location_copy_and_detach()

>
>
> +static inline void rtems_filesystem_add_location_to_mount_entry(
> +  rtems_filesystem_location_info_t *loc
> +)
> +{
> +  rtems_filesystem_mt_entry_lock_context( lock_context );
> +
> +  rtems_filesystem_mt_entry_lock( lock_context );
> +  rtems_chain_append_unprotected(
> +&loc->mt_entry->location_chain,
> +&loc->mt_entry_node
> +  );
> +  rtems_filesystem_mt_entry_unlock( lock_context );
> +}
> +
> +void rtems_filesystem_remove_location_from_mount_entry(
> +  rtems_filesystem_location_info_t *loc
> +);
> I suggest rtems_filesystem_location_add_to_mt_entry/remove_from_mt_entry.

Fixed.

>
>
> +static inline bool rtems_filesystem_is_root_node(
> +  const rtems_filesystem_location_info_t *loc
> +)
> I suggest filesysytem_location_is_root?

Fixed.

>
>
> +static inline void rtems_filesystem_eval_path_pass_back_token(
> +  rtems_filesystem_eval_path_context_t *ctx
> +)
> oddly named function "pass_back_token". I might suggest "put" instead of
> "pass" as more symmetrically named with eval_path_get_next_token. Since this is
> just a static helper function the name is no big deal.

Fixed.

>
> +int rtems_filesystem_exist_in_same_file_system_instance(
> +  const rtems_filesystem_location_info_t *a,
> +  const rtems_filesystem_location_info_t *b
> +);
> maybe rtems_filesystem_location_exists_in_same_fs_instance_as?

Fixed.

>
>
> +static inline bool rtems_filesystem_is_directory_up(
> +  const char *token,
> +  size_t tokenlen
> +)
> maybe rtems_filesystem_is_parent_directory()?

Not sure.

>
>
> +++ b/cpukit/libcsupport/src/__usrenv.c
> should the null handlers defined here set the errno?

No! They must not touch the errno.  Some error will set the errno and the 
purpose of the null handlers is to deliver the error return status.

>
>
> +rtems_filesystem_global_location_t rtems_filesystem_global_location_null = {
> +  .location = {
> +    .mt_entry_node = {
> +      .next =&rtems_filesystem_null_mt_entry.location_chain.Tail.Node,
> +      .previous =&rtems_filesystem_null_mt_entry.location_chain.Head.Node
> +    },
> +    .handlers =&rtems_filesystem_null_handlers,
> +    .ops =&null_ops,
> +    .mt_entry =&rtems_filesystem_null_mt_entry
> +  },
> +  .reference_count = 4
> +};
> why reference_count 4?

The current and root directory in the environment (2), the root node of the 
null file system instance (1), and the mount point node (1).

>
>
> +++ b/cpukit/libcsupport/src/_rename_r.c
>
> +  /* FIXME: This is not POSIX conform */
> +  int new_eval_flags = RTEMS_LIBIO_FOLLOW_HARD_LINK
> +    | RTEMS_LIBIO_MAKE
> +    | RTEMS_LIBIO_EXCLUSIVE;
> FIXME :) What would be conformant?

Please look here:

http://pubs.opengroup.org/onlinepubs/007904875/functions/rename.html

Currently the rename is completely broken (also lseek and truncate).

>
>
> +++ b/cpukit/libcsupport/src/chdir.c
>
> +int rtems_filesystem_chdir( rtems_filesystem_location_info_t *loc )
> ...
> +  } else {
> +    rv = -1;
> +  }
> +
> +  if ( rv != 0 ) {
> +    rtems_filesystem_global_location_release( global_loc );
> +  }
> set errno when setting rv = -1?

No, it is set in rtems_filesystem_global_location_transform().

>
>
> +++ b/cpukit/libcsupport/src/chroot.c
>
> +  /*
> +   * We use the global environment for path evaluation.  This makes it possible
> +   * to escape from a chroot environment referencing an unmounted file system.
> +   */
> clarify comment. Is this a problem that can/should be fixed?

No, this is a feature.  Otherwise you are trapped in a chroot environment if 
someone unmounts the file system, e.g. someone uses an USB stick and does a 
chroot into a fs of this stick.

>
>
> +      sc = rtems_libio_set_private_env();
> +      if (sc == RTEMS_SUCCESSFUL) {
> +        rtems_filesystem_global_location_assign(
> +&rtems_filesystem_root,
> +          new_root_loc
> +        );
> +        rtems_filesystem_global_location_assign(
> +&rtems_filesystem_current,
> +          new_current_loc
> +        );
> +      } else {
> +        if (sc != RTEMS_UNSATISFIED) {
> +          errno = ENOMEM;
> +        }
> +        rv = -1;
> +      }
> what if sc is RTEMS_UNSATISFIED what should errno be?
> ...
> +  } else {
> +    rv = -1;
> set errno?
> +  }
> +  rtems_filesystem_eval_path_cleanup(&ctx );

The environment functions are a bit special since they sometimes set the errno.

>
>
> +  /*
> +   * FIXME: We ignore the start value fd2 for the file descriptor search.  This
> +   * is not POSIX conform.
> +   */
> FIXME :) How to make it conform?

This is up to another patch.  No new features in this patch.

>
>
> +++ b/cpukit/libcsupport/src/freenode.c
> +void rtems_filesystem_freenode( rtems_filesystem_location_info_t *node )
> Might be worth calling this rtems_filesystem_location_free()?

Hm, this name is historic.  Not sure.

>
>
> +++ b/cpukit/libcsupport/src/ftruncate.c
>
> +    rv = (*iop->pathinfo.handlers->ftruncate_h)( iop, length );
> why not locking this operation?

The lock currently only covers the file system operations not the node 
handlers.  This is another issue.  See

https://www.rtems.org/bugzilla/show_bug.cgi?id=1255

>
>
> +++ b/cpukit/libcsupport/src/libio_sockets.c
>
> +#include<rtems/rtems_bsdnet_internal.h>
> why including this?

Remove it and look for warnings.

>
>
> +++ b/cpukit/libcsupport/src/lseek.c
>
> +  new_offset = reference_offset + offset;
> Something wrong here. reference_offset undefined if default case hits.

Sorry, I fixed this already, but there is more broken than that, see

https://www.rtems.org/bugzilla/show_bug.cgi?id=2027

>
>
> +      rv = (*iop->pathinfo.handlers->lseek_h)( iop, offset, whence );
> why not locking this operation?

See above.

>
>
> +++ b/cpukit/libcsupport/src/mount.c
> @@ -92,6 +81,23 @@ static rtems_filesystem_mount_table_entry_t
> *alloc_mount_table_entry(
> +    rtems_chain_initialize(
> +&mt_entry->location_chain,
> +      mt_fs_root,
> +      1,
> +      sizeof(*mt_fs_root)
> +    );
> +  } else {
> +    free( mt_entry );
> +  }
> This is probably wrong since mt_entry would be NULL here.
> Should set errno=ENOMEM

Sorry, I changed this in the meantime.

>
>
> +static int register_subordinate_file_system(
> + rtems_filesystem_mount_table_entry_t *mt_entry,
> + const char *target
> should be two leading spaces.

Fixed.

>
>
> +int mount(
> ...
> +            (*mt_entry->mt_fs_root->location.ops->fsunmount_me_h)( mt_entry );
> why not locking this operation?

The mount() and unmount () of on file system instance must be done by at most 
one thread at a time.

>
>
> +        if ( rv != 0 ) {
> +          free( mt_entry );
> +        }
> set errno?

Set elsewhere.

>
>
> +++ b/cpukit/libcsupport/src/open.c
>
> +static int do_open(
> ...
> +  rv = (*iop->pathinfo.handlers->open_h)( iop, path, oflag, mode );
> why not locking this operation?
>
>
> +      if ( rv != 0 ) {
> +        (*iop->pathinfo.handlers->close_h)( iop );
> +      }
> why not locking this operation?

See above.

>
>
>
> +static void free_user_env(void *arg)
> ...
> +  /*
> +   * It is possible to see the global environment here if someone deletes the
> +   * task right after the task variable add in rtems_libio_set_private_env().
> +   */
> make it more clear this is protected by thread dispatching disable

What is protected?

>
>
> +++ b/cpukit/libcsupport/src/sup_fs_location.c
>
> +void rtems_filesystem_do_unmount(
> +  (*mt_entry->mt_fs_root->location.ops->fsunmount_me_h)(mt_entry);
> why not locking this operation?

See above.

>
>
> +++ b/cpukit/libcsupport/src/unmount.c
>
> +int unmount( const char *path )
> ...
> +    rv = (*mt_entry->mt_point_node->location.ops->unmount_h)( mt_entry );
> +    if ( rv == 0 ) {
> +      rtems_filesystem_mt_entry_lock_context( lock_context );
> +
> +      rtems_filesystem_mt_entry_lock( lock_context );
> +      mt_entry->mounted = false;
> +      rtems_filesystem_mt_entry_unlock( lock_context );
> +    }
> why lock here? isn't this protected by eval_path_start instance lock?

The lock could be a real lock and not simply an interrupt disable/enable.  I 
want to make sure that the state is consistent within the critical section.

>
>
> +++ b/cpukit/libfs/src/devfs/devfs.h
> /**
> + *  The following defines the device-only filesystem operating
> + *  handlers.
>    */
> imprecise comment i think these are the
> device-only filesystem file operating handlers.
> +extern const rtems_filesystem_file_handlers_r  devFS_file_handlers;

Fixed.

>
>
> +++ b/cpukit/libfs/src/imfs/imfs_rename.c
> +  /*
> +   * FIXME: Due to insufficient checks we can create inaccessible nodes with
> +   * this operation.
> +   */
> FIXME :) Please explain a little more.

You can rename a parent into a subdirectoy of its child.

>
>
> +++ b/cpukit/libfs/src/imfs/ioman.c
>   rtems_status_code rtems_io_lookup_name(
>     const char           *name,
>     rtems_driver_name_t  *device_info
>   )
> now that this is not dependent on IMFS(?) should it be moved

Not sure.

>
> +++ b/cpukit/libfs/src/nfsclient/src/nfs.c
> while making these changes maybe fix the indent level of nfs functions?
> A lot of these nfs functions are not indented according to our style guide

Yes, the format is special.  I try (its hard) to keep the current style of a file.

>
>
> @@ -2035,7 +1799,8 @@ LOCK(nfsGlob.llock);
> +                rtems_fatal_error_occurred(0xdeadbeef);
> +                return;
> unreached return? fix indent level.

Not in this patch.

>
>
> @@ -3096,7 +2730,7 @@ Nfs		nfs;
> +		if (rtems_filesystem_resolve_location(mntpt, MAXPATHLEN,
> &nfs->mt_entry->mt_fs_root->location))
> line length>80

Not in this patch (there are so many lines > 80 in this file).

>
> +++ b/cpukit/libfs/src/rfs/rtems-rfs-rtems.c
>
> @@ -707,73 +400,46 @@
> rtems_rfs_rtems_utime(rtems_filesystem_location_info_t* pathloc,
>   /**
>    * The following rouine creates a new symbolic link node under parent with the
> + * name given in name.
> fix (preexisting) typo s/rouine/routine/.
> Also change s/name given in name/name given in node_name/.

Fixed.

>
>
> @@ -1304,19 +927,17 @@ rtems_rfs_rtems_initialise
> (rtems_filesystem_mount_table_entry_t* mt_entry,
>
>   rtems_rfs_rtems_shutdown (rtems_filesystem_mount_table_entry_t* mt_entry)
> +  /* FIXME: Return value? */
> +  rtems_rfs_fs_close(fs);
> FIXME :) Just eliminate comment, since rfs_fs_close appears always
> to return 0.

And in the future?  We may change it to void, add an assert here or use a fatal 
error.

>
>
> +++ b/cpukit/libmisc/shell/main_lsof.c
>
> +      printk(
> +        "\t0x%08x ->  0x%08x\n",
> +        loc,
> +        loc->node_access
> +      );
> just put on one line?

This is work in progress.

>
>
> +++ b/cpukit/libnetworking/lib/ftpfs.c
>
> +static void rtems_ftpfs_eval_path(
> +  rtems_filesystem_eval_path_context_t *self
>   )
> Why doesn't this use the rtems_filesystem_eval_path_generic code?

No new features in this patch.

>
>
> +++ b/cpukit/libnetworking/lib/tftpDriver.c
> +  fs = malloc (sizeof (*fs));
> +  root_path = malloc (devicelen + 2);
> +  if (root_path == NULL || fs == NULL)
> +      goto error;
> should just rtems_set_errno_and_return_minus_one (ENOMEM).
>
> +
> +error:
> +
> +  free (fs);
> +  free (root_path);
> +
> +  rtems_set_errno_and_return_minus_one (ENOMEM);
> this is wrong if jumping here because fs==NULL or root_path==NULL

free(NULL) is a non-operation.

>
>
> +static void rtems_tftp_eval_path(rtems_filesystem_eval_path_context_t *self)
> Why doesn't this use the rtems_filesystem_eval_path_generic code?

No new features in this patch.

>
> +        if (
> +            eval_flags == 0
> +                || eval_flags == RTEMS_LIBIO_PERMS_READ
> +                || eval_flags == RTEMS_LIBIO_PERMS_WRITE
> +        ) {
> as with tfs is this equivalent to (eval_flags != RDW)?

Fixed.

>
>
> +                memcpy(
> +                    current + currentlen,
> +                    path,
> +                    pathlen
> +                );
> put on one line?

Fixed.

Thanks a lot Gedare.

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.