| Message ID | 20191227085335.10144-1-zajec5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [OpenWrt-Devel,fstools] Revert "block: mount_action: handle mount/umount deps" | expand |
On Fri, 27 Dec 2019 at 16:53, Rafał Miłecki <zajec5@gmail.com> wrote: > > From: Rafał Miłecki <rafal@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 > > 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. Regards, yousong
On 27.12.2019 13:25, Yousong Zhou wrote: > On Fri, 27 Dec 2019 at 16:53, Rafał Miłecki <zajec5@gmail.com> wrote: >> >> From: Rafał Miłecki <rafal@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' option uuid 'BBBB' option enabled '1' option autofs '1'
On Sat, 28 Dec 2019 at 18:27, Rafał Miłecki <zajec5@gmail.com> wrote: > > On 27.12.2019 13:25, Yousong Zhou wrote: > > On Fri, 27 Dec 2019 at 16:53, Rafał Miłecki <zajec5@gmail.com> wrote: > >> > >> From: Rafał Miłecki <rafal@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'
diff --git a/block.c b/block.c index b6d49a8..e07cbc5 100644 --- a/block.c +++ b/block.c @@ -1199,47 +1199,30 @@ static int umount_device(char *path, int type, bool all) static int mount_action(char *action, char *device, int type) { - struct device *the_dev, *dev; char path[32]; if (!action || !device) return -1; - - if (config_load(NULL)) - return -1; - cache_load(0); - - the_dev = find_block_device(NULL, NULL, device); + snprintf(path, sizeof(path), "/dev/%s", device); if (!strcmp(action, "remove")) { if (type == TYPE_HOTPLUG) blockd_notify(device, NULL, NULL); - if (!the_dev || !the_dev->m || the_dev->m->type != TYPE_MOUNT) { - snprintf(path, sizeof(path), "/dev/%s", device); - umount_device(path, type, true); - } else - vlist_for_element_to_last_reverse(&devices, the_dev, dev, node) - if (dev->m && dev->m->type == TYPE_MOUNT) - umount_device(dev->pr->dev, type, true); + umount_device(path, type, true); + return 0; - } else if (!strcmp(action, "add")) { - if (!the_dev) - return -1; - if (the_dev->m && the_dev->m->type == TYPE_MOUNT) { - vlist_for_first_to_element(&devices, the_dev, dev, node) { - if (dev->m && dev->m->type == TYPE_MOUNT) { - int err = mount_device(dev, type); - if (err) - return err; - } - } - return 0; - } else - return mount_device(the_dev, type); + } else if (strcmp(action, "add")) { + ULOG_ERR("Unkown action %s\n", action); + + return -1; } - ULOG_ERR("Unkown action %s\n", action); - return -1; + + if (config_load(NULL)) + return -1; + cache_load(0); + + return mount_device(find_block_device(NULL, NULL, path), type); } static int main_hotplug(int argc, char **argv)