[systemd-devel] [PATCH] Fix starting swap unit on symlink made it unstoppable

[jjacky]

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 (i.am.jack.mail at gmail.com) 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 <i.am.jack.mail at gmail.com>
>>>> ---
>>>> 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