lib/libc/unmount.c

[Eugeny S. Mints]

On Thu, 11 Oct 2001, Till Straumann wrote:

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

In general rtems_filesystem_freenode() frees ONLY node_acces
field of fs_root_loc structure( it may frees handler and ops fields, but
never mt_entry) - more over it never can free
mt_entry field because it was malloced by libc code. So,
access fields of fs_root_loc after having released is
valid according to ideology.

> 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;
> }
>
>

Please see carefully - it is already in the patch.

>
> >
> >
> > 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;
> }

In was too long:), but you right at the end: in unmount.c only one call to
rtems_filesystem_freenode(&fs_root_loc) - corresponding to
rtems_filesystem_evaluate_path() in file_systems_below_this_mountpoint -
should present (this is the main ideology that was described in the letter I
forwarded to you: for any rtems_filesystem_evaluate_path call should exists
rtems_filesystem_freenode call !)
'Second freeing' about what you worry  should go into my_fsunmount_me_h
(it is not necessay to be direct
rtems_filesystem_freenode(&mt_entry->mt_fs_root) call - in general it will
be direct my_freenod_h call ( the function macro rtems_filesystem_freenode maps
to )

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

-- 
Eugeny S. Mints
OKTET Ltd.
1 Ulianovskaya st., Petergof, St.Petersburg, 198904 Russia
Phone: +7(812)428-4384 Fax: +7(812)327-2246
mailto:Eugeny.Mints at oktet.ru