On 10/29/12 11:03, Zbigniew Jędrzejewski-Szmek wrote: > On Tue, Oct 23, 2012 at 10:15:00PM +0200, jjacky wrote: >> >> On 10/23/12 21:05, Lennart Poettering wrote: >>> On Sat, 13.10.12 14:24, Olivier Brunel ([email protected]) wrote: >>> >>>> Starting a swap unit pointing to (What) a symlink (e.g. /dev/mapper/swap >>>> or /dev/disk/by-uuid/...) would have said unit be marked active, follow >>>> the one using the "actual" device (/dev/{dm-1,sda3}), but that new unit >>>> would be seen as inactive. >>>> Since all requests to stop swap units would follow/redirect to it, >>>> and it is seen inactive, nothing would be done (swapoff never called). >>>> >>>> This is because this unit would be treated twice in >>>> swap_process_new_swap, the second call to swap_add_one causing it to >>>> eventually be marked inactive. >>>> >>>> Signed-off-by: Olivier Brunel <[email protected]> >>>> --- >>>> The patch removes the call to udev_device_get_devnode, assuming that >>>> device nodes (and not symlinks) are used under /proc/swaps, which >>>> seems to be the case. >>> >>> This is not the case on Fedora at least. >> >> Oh? I assumed it would be more of a kernel thing, so likely to be the >> same everywhere. Also, a quick test in a VM running FC17, when I enable >> a swap on /dev/mapper/foo it gets listed as /dev/dm-2 under /proc/swaps, >> aka where the symlink points to. >> Anyhow... >> >>> >>> swap.c really needs to handle "following" devices properly (see other >>> mail). We cut some corners there, and we shouldn't. >> >> Does that mean keeping the call to udev_device_get_devnode and simply >> using strcmp to make sure the same unit isn't processed twice (which >> causes the bug) isn't enough? >> >> At least AFAICS it does fix the bug, and the "following" seems to be >> working fine. > Hi, > I now pushed an ammended version of your patch, which, > like you suggested, does a string compare on the name to avoid > calling swap_add_one twice with the same name. This definitely > helps in some cases.
Just a little follow-up, because the patch committed only adds a test for the first call to swap_add_one, but not the one for symlinks. Or, if symlinks can also be listed under /proc/swaps then we're still calling swap_add_one twice with the same name; which is why I had send another patch[1] adding the second test. > Nevertheless, it is still possible to confuse the swap logic without too > much trouble. E.g. doing > > systemctl start dev-sda3.swap 'dev-disk-by\x2dlabel-SWAP.swap' > > when they both refer to the same thing, causes one of the jobs to > fail. Start jobs are normally idempotent, so this case shouldn't fail > either. Additionally, I sent a patch about this[2] which, I think, should resolves such issues. It makes swap units pointing to symlinks follow the swap unit of the devnode when swap is off (i.e. they're not already following the one from /proc/swaps). So when that's also what shows up on /proc/swaps, you always have the same following in place, whether swap is on or off. If a symlinks show up in /proc/swaps however, you get different followings, but that shouldn't be a problem. > > Zbyszek > -j [1] http://lists.freedesktop.org/archives/systemd-devel/2012-October/007235.html [2] http://lists.freedesktop.org/archives/systemd-devel/2012-October/007236.html _______________________________________________ systemd-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/systemd-devel
