[OpenWrt-Devel] [PATCH fstools] Revert "block: mount_action: handle mount/umount deps"

[Yousong Zhou]

On Sat, 28 Dec 2019 at 18:27, Rafał Miłecki <zajec5 at gmail.com> wrote:
>
> On 27.12.2019 13:25, Yousong Zhou wrote:
> > On Fri, 27 Dec 2019 at 16:53, Rafał Miłecki <zajec5 at gmail.com> wrote:
> >>
> >> From: Rafał Miłecki <rafal at milecki.pl>
> >>
> >> This reverts commit 32c3126b2f0464106d74317336b6aef1d7d5f82f.
> >>
> >> Internal list of devices guarantees some basic sorting (by device type
> >> and then a name) but nothing more. In particular it's not guaranteed
> >> (and it's actually quite uncommon) that all preceding entries are parent
> >> devices.
> >>
> >> Mounting all preceding devices may easily result in unrelated mounts.
> >> They can fail easily basically breaking original mounting attempt, e.g.:
> >>
> >> daemon.err blockd: kernel is requesting a mount -> sda2
> >> daemon.err block: /dev/sda1 is already mounted on /tmp/run/blockd/sda1
> >> daemon.err block: autofs: "add" action has failed: -1
> >> daemon.err blockd: failed to run block. add/sda2
> >
> > Sorry for the inconvenience.  But the error (regression) should be
> > caused by 2f2a09ad ("block: mount_device: err log only when mp
> > deviates from spec").  m->target is expected to match the actual mount
> > point only when it is not managed by blockd (m->autofs).
> >
> > Please see if the following patch works.  We can also check if m is
> > managed by autofs and m->target a symlink whose target matches mp, but
> > that's a bit overkill.
> >
> > --- a/block.c
> > +++ b/block.c
> > @@ -1100,7 +1100,7 @@ static int mount_device(struct device *dev, int type)
> >
> >          mp = find_mount_point(pr->dev);
> >          if (mp) {
> > -               if (m && m->type == TYPE_MOUNT && strcmp(m->target, mp)) {
> > +               if (m && !m->autofs && m->type == TYPE_MOUNT && strcmp(m->target, mp)) {
> >                          ULOG_ERR("%s is already mounted on %s\n", pr->dev, mp);
> >                          err = -1;
> >                  } else
> >
>
> You're right about that error and your diff indeed fixes that:
> /dev/sda1 is already mounted on /tmp/run/blockd/sda1
> for me. It still doesn't fix mounting unneeded devices though.
>
>
> Please check this:
>
> # mount | grep "/dev/sd"
>
> # ls /var/run/blockd/sdb2
> b.txt
>
> # mount | grep "/dev/sd"
> /dev/sda1 on /tmp/run/blockd/sda1 type vfat (rw,relatime,fmask=0000,dmask=0000,allow_utime=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
> /dev/sda2 on /tmp/run/blockd/sda2 type vfat (rw,relatime,fmask=0000,dmask=0000,allow_utime=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
> /dev/sdb1 on /tmp/run/blockd/sdb1 type fuseblk (rw,relatime,user_id=0,group_id=0,allow_other,blksize=4096)
> /dev/sdb2 on /tmp/run/blockd/sdb2 type fuseblk (rw,relatime,user_id=0,group_id=0,allow_other,blksize=4096)
>
> As you can see, accessing "sdb2" partition resulted in mounting 3 other
> partitions that I don't need at all. Including spinning 1 unused disk!
>
>
> >> If some dependency handling is required it should be implemented
> >> explicitly as current solution isn't reliable and it breaks autofs when
> >> using multiple devices (partitions).
> >>
> >
> > Dependencies are directly implied by mount target as specified in the
> > uci config file.  This relationship is inherently there.  E.g.
> >
> >   1. mount target /mnt/a
> >   2. mount target /mnt/a/b
> >
> > Then "1" must mount before "2".  "2" before "1" is not practically
> > useful in any way.
>
> That relationship/dependency isn't directly parsed by fstools in any
> way. By making it explicit I thought of something like:
>
> config 'mount' '/dev/vdc'
>         option  target  '/mnt'
>         option  uuid    'AAAA'
>         option  enabled '1'
>         option  autofs  '1'
>
> config 'mount' '/dev/vdb'
>         option  parent  '/dev/vdc/
>         option  target  '/mnt/s'

The point is that this "parent" relationship is already implied.  This
extra uci option does add more information.

> That relationship/dependency isn't directly parsed by fstools in any
> way.

That devces list was modified to be ordered primarily by length of
mount target was for this (fb0700f0 "block: support hierarchical
mount/umount").  But it's not tight/precise enough to only mount
direct dependencies, but all enabled mounts whose length smaller than
itself.

I made a patch [1] few days ago trying to amend that but didn't yet
have time to test it.  Now I checked it again, that "target" defaults
to "/mnt/<dev>"  and anon_mount=1 also need to be taken into
consideration.  Nevertheless, this dependency tracking mechanism still
needs to be handled by the program itself, not extra knobs.

[1] https://github.com/yousong/fstools/commit/0eab853a7857c967efbc6fb7a6e0ba20e5dd3aba

Regards,
                yousong

>         option  uuid    'BBBB'
>         option  enabled '1'
>         option  autofs  '1'

_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel