lib/libc/unmount.c

[Till Straumann]

Hi all.

Eugeny is right - we should get back at these FS issues.

"Eugeny S. Mints" wrote:

> Hi, All!
>
> Could you please comment rtems_filesystem_freenode() call( line 132 in
> lib/libc/unmount.c) for &fs_root_loc after
> temp_mt_entry.mt_fs_root.ops->fsunmount_me_h() call( line 115 in
> lib/libc/unmount.c)?( I use rtems-ss-20010816)
>
> As I understand, fsunmount_me_h() provides to mounted fs a chance to make full
> clean up activity (i.e. to free all memory, delete semaphores if mounted fs used
> any, etc). If it right, then after this moment nobody can call any internal
> routines of mounted(but really already unmounted at this moment) fs, am I
> right? But rtems_filesystem_freenode does.

IMO, your suggestion (patch) regarding "unmount.c" makes a valid point:
The "fs_root_loc" node must be released _before_ calling the 'fsunmount_me_h()'
method.

However, I don't like very much that you still access fields of fs_root_loc
after having released it. Although it seems safe, IMHO it's not very
elegant...
Also, I don't like the fact that a FS implementation has no way of
rejecting to being unmounted and I'd rather do the unmount
in a 'bottom-up' fashion, i.e. cleanup the mounted FS _before_
detaching it from the mount point thus giving the mounted FS
a chance to bail out. Hence I propose something like this (libc/unmount.c):

int unmount(char *path) {
 ...
/* prepare unmounting as usal */
 ...

mt_root = fs_root_loc.mt_entry;  /* copy mt_entry handle */
rtems_filesystem_freenode(&fs_root_loc); /* release root node */

/* now start unmounting the filesystem in a 'bottom-up' fashion */
status = temp_mt_entry.mt_fs_root.ops->fsunmount_me_h(mt_root);

if (UMOUNT_REJECTED == status) return -EBUSY; /* FS is still mounted and usable */

/* proceed unmounting, even if fsunmount_me_h failed for some other reason */
if (tmp_mt_entry.mt_point_node.ops->fsunmount_h(mt_root)) {
    status = -1;
}

... /* continue as usual, i.e. cleaning up "mt_root"
     * NOTE: it is the FS implementation's responsibility
     * to call rtems_filesystem_freenode(&mt_root->mt_fs_root)
     * from fsunmount_me_h()!
     */

return status;
}


>

-- snip --


>
>
> Anyway, it seems the main idea is that nobody can call any
> internal routines of an fs after fsunmount_me_h() call for
> this fs.

This is now respected.

>
>
> As to Mr. Till Straumann patch (attached to the letter
> "fs core fixes (was Re: TFTP driver changes)"  Date: Wed, 26 Sep 2001
> 15:40:32 -0700): I didn;t look carefully all files he proposed to patch, but
> as to umnount.c, it seems that in line 134 of patched unmount.c he tries to
> free the same as in line 136, because it seems to me, that
> &fs_root_loc.mt_entry->mt_fs_root and &fs_root_loc points to the same memory
> area.

It does definitely _not_ point to the same memory area and hence _not_
to the same location_info_t structure, although both structs "refer" to the
"same thing", namely the mounted FS' root. Nevertheless, both location_info_t
structs need to be properly released.
'fs_root_loc' is a location_info_t struct on the stack existing within the scope of
the 'unmount()' routine (It was allocated by evaluate_path at the top of
unmount()).
On the other hand, fs_root_loc.mt_entry->mt_fs_root is part of the
(*fs_root_loc.mt_entry), i.e. occupying memory that was allocated from the free
store
when mounting the fs. At the same time, the mt_entry->mt_fs_root node had been
initialized.
Note that this very piece of memory (*fs_root_loc.mt_entry) is about to be 'free'ed

at the end of 'unmount()'.

(BTW: The terms 'allocate' and 'free' when talking about location_info_t structs
are
slightly misleading because non memory allocation/deallocation is actually
involved.)

Consider the following:

1) "mt_entry->mt_fs_root" is a 'valid' location_info_t structure which needs
    to be released by calling rtems_filesystem_freenode() somewhere.
2) as stated above, freenode() must not be called after the FS has (partially)
    been unmounted by fsunmount_me_h().

Hence, the conclusion is that it is the _responsibility_of_a_FS_implementation_
to call rtems_filesystem_freenode() on its mt_entry argument's mt_fs_root
field if it doesn't reject the unmount.

I.e. a correct FS implementation's fsunmount_me_h() _must_ adhere to the
policy:

static int
my_fsunmount_me_h(rtems_filesystem_mount_table_entry_t *mt_entry)
{
  if ( _I_reject_being_unmounted_for_whateverreason)
        return UMOUNT_REJECTED;
  start_cleanup();
  /* release the critical node as long as it's still safe to
   * call freenode and as soon as we don't need it anymore.
   */
  rtems_filesystem_freenode(&mt_entry->mt_fs_root);
  finish_cleanup();
  return status;
}

> At the same time additions rtems_filesystem_freenode(fs_root_loc) in
> lines 177 and 199 of patched file seem right.
>

They still seem right to myself...

Again, sorry for the long message, but this is a little bit involved.


Regards

--Till.

PS I didn't bother to create a patch because I think we should discuss
this first.