Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Lennart Poettering 
On Tue, 19.05.15 17:31, Martin Pitt (martin.p...@ubuntu.com) wrote:

> Hey Lennart,
> 
> Lennart Poettering [2015-05-19 17:23 +0200]:
> > Yes, we need to also record those which were just mounted or changed,
> > in case systemd rereads /proc/self/mountinfo at a time where two
> > mounts where created at once from the same device.
> 
> OK, as I suspected.
> 
> > I now committed a change to implement that, can you check if this
> > fixes the issue for you?
> 
> It does, and it looks much more straightforward now.

Perfect! Thanks for tracking this down and keeping it up even when I
was too dumb to understand what the issue was!

Thanks!

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Martin Pitt 
Hey Lennart,

Lennart Poettering [2015-05-19 17:23 +0200]:
> Yes, we need to also record those which were just mounted or changed,
> in case systemd rereads /proc/self/mountinfo at a time where two
> mounts where created at once from the same device.

OK, as I suspected.

> I now committed a change to implement that, can you check if this
> fixes the issue for you?

It does, and it looks much more straightforward now.

Many thanks for your patience, this was a hairy thing to untangle..

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)


signature.asc
Description: Digital signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Lennart Poettering 
On Tue, 19.05.15 16:05, Martin Pitt (martin.p...@ubuntu.com) wrote:

> That was a red herring. Turns out that your patch only added devices
> to "around" which had "mount->just_mounted || mount->just_changed",
> which is obviously not the case for mounts which have been around for
> a while (and these are the ones we need to keep!). This patch fixes
> that; although I'm unsure yet whether we *also* need to record the ones
> with "mount->just_mounted || mount->just_changed"; my gut feeling says
> yes, and we need to restructure this a bit to run the set_put(around, ...)
> for all mount->is_mounted Mount units. WDYT?

Yes, we need to also record those which were just mounted or changed,
in case systemd rereads /proc/self/mountinfo at a time where two
mounts where created at once from the same device.

I now committed a change to implement that, can you check if this
fixes the issue for you?

Thanks!

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread systemd github import bot 
Patchset imported to github.
Pull request:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Martin Pitt 
Martin Pitt [2015-05-19 15:17 +0200]:
> My first hunch is that this is caused by calling
> mount_load_proc_self_mountinfo() in mount_dispatch_io()
> (src/core/mount.c:1682) *before* it goes through that new
> SET_FOREACH() loop. That call will already see the removed mount and
> call device_found_node() with the removal.

That was a red herring. Turns out that your patch only added devices
to "around" which had "mount->just_mounted || mount->just_changed",
which is obviously not the case for mounts which have been around for
a while (and these are the ones we need to keep!). This patch fixes
that; although I'm unsure yet whether we *also* need to record the ones
with "mount->just_mounted || mount->just_changed"; my gut feeling says
yes, and we need to restructure this a bit to run the set_put(around, ...)
for all mount->is_mounted Mount units. WDYT?

Anyway, with this it behaves as it should.

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
From fb089bb6dfccd63b395fbda8461bba6ad3541827 Mon Sep 17 00:00:00 2001
From: Martin Pitt 
Date: Tue, 19 May 2015 15:55:23 +0200
Subject: [PATCH] mount: Fix mountinfo check for devices which are still around

Fix commit fcd8b266: For recording the devices for which there are still mounts
around in mountinfo we need to consider the mounts which did *not* just get
mounted or changed, but actually have existed for a longer time.

TODO: check if we *also* need to consider mounts with just_{mounted,changed}
---
 src/core/mount.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/core/mount.c b/src/core/mount.c
index c1a9ea5..06e0807 100644
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -1751,12 +1751,11 @@ static int mount_dispatch_io(sd_event_source *source, int fd, uint32_t revents,
 break;
 }
 
-if (mount->parameters_proc_self_mountinfo.what) {
-
-if (set_ensure_allocated(&around, &string_hash_ops) < 0 ||
-set_put(around, mount->parameters_proc_self_mountinfo.what) < 0)
-log_oom();
-}
+} else if (mount->state != MOUNT_DEAD && mount->state != MOUNT_FAILED &&
+   mount->parameters_proc_self_mountinfo.what) {
+if (set_ensure_allocated(&around, &string_hash_ops) < 0 ||
+set_put(around, mount->parameters_proc_self_mountinfo.what) < 0)
+log_oom();
 }
 
 /* Reset the flags for later calls */
-- 
2.1.4



signature.asc
Description: Digital signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Umut Tezduyar Lindskog 
On Tue, May 19, 2015 at 1:56 PM, Lennart Poettering
 wrote:
> On Tue, 19.05.15 10:23, Martin Pitt (martin.p...@ubuntu.com) wrote:
>
>> Hello all,
>>
>> I have a better fix now.
>>
>> > So the problem is that this tentative → dead transition only works if
>> > a device is referenced once, but causes overzealous unmounts if there
>> > are more references.
>
> Ah, indeed, that makes sense!
>
>> > We need a reference count, or check if the device is bound by other
>> > device still. Come to think of it now, we actually already have that:
>> >
>> >   Id=dev-foo.device
>> >   BoundBy=tmp-boot.mount tmp-etc.mount
>> >
>> > But my patch doesn't take that into account yet.
>>
>> This one does now. I left a fairly comprehensive commit log as this
>> isn't that easy to explain/understand. If it's too verbose for you I
>> can also trim it back a bit.
>
> I have now committed a different fix now, that keeps counting of the
> mount points in mount.c, instead of "reaching over" from device.c.
>
> I only gave this light testing, would be cool if you could check if
> this fixes things for you.
>
> http://cgit.freedesktop.org/systemd/systemd/commit/?id=fcd8b266edf0df2b85079fcf7b099cd4028740e6
>
> This commit will now collect two sets of devices while going through
> /proc/self/mountinfo: the devices of lines that are no longer there,
> and the devices of lines that are there. Only for devices in the
> former set that are not in the latter we will now propagate an event
> to device.c.
>
> Does this make sense?
>
>> +/* If we found a tentative device via mountinfo which is still
>> + * referenced by other mounts than the one that just got unmounted, 
>> we
>> + * must leave it FOUND_MOUNT/tentative; otherwise we'd trigger
>> + * unmounting of all other bound mounts. */
>> +if (d->found == DEVICE_FOUND_MOUNT && n == 0) {
>> +Iterator i;
>> +Unit *bound;
>> +
>> +SET_FOREACH(bound, UNIT(d)->dependencies[UNIT_BOUND_BY], i) 
>> {
>> +if (bound->type == UNIT_MOUNT &&
>> +MOUNT(bound)->state != MOUNT_DEAD && 
>> MOUNT(bound)->state != MOUNT_FAILED) {
>> +log_unit_debug(UNIT(d), "Still bound by %s 
>> (%s), keeping state.",
>> +   bound->id, 
>> mount_state_to_string(MOUNT(bound)->state));
>> +return;
>> +}
>> +}
>> +log_unit_debug(UNIT(d), "Not referenced by any mounts any 
>> more, changing to dead.");
>> +}
>> +
>>  previous = d->found;
>>  d->found = n;
>
> Lennart

Didn't work for me. Removed device doesn't go in to either cases, so
it never makes it to the "around" set but it makes it to "gone" set.

if (!mount->is_mounted)
{

}
else if (mount->just_mounted || mount->just_changed)
{

}

Umut

>
> --
> Lennart Poettering, Red Hat
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Martin Pitt 
Hey Lennart,

Lennart Poettering [2015-05-19 13:56 +0200]:
> I have now committed a different fix now, that keeps counting of the
> mount points in mount.c, instead of "reaching over" from device.c.
> 
> I only gave this light testing, would be cool if you could check if
> this fixes things for you.
> 
> http://cgit.freedesktop.org/systemd/systemd/commit/?id=fcd8b266edf0df2b85079fcf7b099cd4028740e6
> 
> This commit will now collect two sets of devices while going through
> /proc/self/mountinfo: the devices of lines that are no longer there,
> and the devices of lines that are there. Only for devices in the
> former set that are not in the latter we will now propagate an event
> to device.c.
> 
> Does this make sense?

It does, and it indeed should avoid some round trips. However, it does
not work yet. I added this for extra debugging:

--- a/src/core/device.c
+++ b/src/core/device.c
@@ -771,6 +771,9 @@ int device_found_node(Manager *m, const char *node, bool 
add, DeviceFound found,
 assert(m);
 assert(node);
 
+if (node[0] == '/')
+log_warning(" device_found_node node %s add %i found %i 
now %i", node, add, found, now);
+
 /* This is called whenever we find a device referenced in
  * /proc/swaps or /proc/self/mounts. Such a device might be
  * mounted/enabled at a time where udev has not finished

After unmounting /tmp/etc, my dev-sda3.device (which plays the role of
"dev-foo.device") still becomes "dead", and /tmp/boot gets unmounted:

 unmounting /tmp/etc 
Id=dev-sda3.device
BindsTo=
BoundBy=
ActiveState=inactive
SubState=dead

Id=tmp-boot.mount
BindsTo=
BoundBy=
ActiveState=inactive
SubState=dead

Journal follows. The first bits are from the manual "mount" commands:


| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]: tmp-etc.mount: Changed dead -> mounted
| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]: tmp-boot.mount: Changed dead -> mounted
| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1

Now "umount /tmp/etc" happens:

| systemd[1]: tmp-etc.mount: Changed mounted -> dead
| systemd[1]:  device_found_node node /dev/sda3 add 0 found 2 now 1

^ So device_found_node() already gets called here, although there
should still be another active mount on sda3. This now causes the
usual cleanup unmount slaughter:

| systemd[1]: dev-sda3.device: Changed tentative -> dead
| systemd[1]: tmp-boot.mount: Trying to enqueue job tmp-boot.mount/stop/replace
| systemd[1]: tmp-boot.mount: Installed new job tmp-boot.mount/stop as 357
| systemd[1]: tmp-boot.mount: Enqueued job tmp-boot.mount/stop as 357
| systemd[1]: tmp-etc.mount: Collecting.
| systemd[1]: Failed to reset devices.list on 
/lxc/test/system.slice/tmp-boot.mount: Permission denied

(FTR, I get thousands of those, but that's unrelated)

| systemd[1]: tmp-boot.mount: About to execute: /bin/umount /tmp/boot -n
| systemd[1]: tmp-boot.mount: Forked /bin/umount as 641
| systemd[1]: tmp-boot.mount: Changed mounted -> unmounting
| systemd[1]: Unmounting /tmp/boot...
| systemd[641]: tmp-boot.mount: Executing: /bin/umount /tmp/boot -n

I'm not sure where this comes from now; there is no manual mount
command to bring back /tmp/boot. It looks like it "bounces", and
quickly remounts /tmp/boot and then unmounts it again:

| systemd[1]:  device_found_node node /dev/sda3 add 1 found 2 now 1
| systemd[1]: dev-sda3.device: Changed dead -> tentative
| systemd[1]: tmp-boot.mount: Trying to enqueue job tmp-boot.mount/start/fail
| systemd[1]: Requested transaction contradicts existing jobs: Resource 
deadlock avoided
| systemd[1]:  device_found_node node /dev/sda3 add 0 found 2 now 1
| systemd[1]: dev-sda3.device: Changed tentative -> dead
| systemd[1]: Received SIGCHLD from PID 641 (umount).
| systemd[1]: Child 641 (umount) died (code=exited, status=0/SUCCESS)
| systemd[1]: tmp-boot.mount: Child 641 belongs to tmp-boot.mount
| systemd[1]: tmp-boot.mount: Mount process exited, code=exited status=0
| systemd[1]: tmp-boot.mount: Changed unmounting -> dead
| systemd[1]: tmp-boot.mount: Job tmp-boot.mount/stop finished, result=done
| systemd[1]: Unmounted /tmp/boot.
| systemd[1]: tmp-boot.mount: Collecting.
| systemd[1]: dev-sda3.device: Collecting.

My first hunch is that this is caused by calling
mount_load_proc_self_mountinfo() in mount_dispatch_io()
(src/core/mount.c:1682) *before* it goes through that new
SET_FOREACH() loop. That call will already see the removed mount and
call device_found_node() with the removal.

I'll debug this more closely ASAP, just need to finish something else
first.

Martin
-- 
Martin Pitt

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread Lennart Poettering 
On Tue, 19.05.15 10:23, Martin Pitt (martin.p...@ubuntu.com) wrote:

> Hello all,
> 
> I have a better fix now.
> 
> > So the problem is that this tentative → dead transition only works if
> > a device is referenced once, but causes overzealous unmounts if there
> > are more references.

Ah, indeed, that makes sense!

> > We need a reference count, or check if the device is bound by other
> > device still. Come to think of it now, we actually already have that:
> > 
> >   Id=dev-foo.device
> >   BoundBy=tmp-boot.mount tmp-etc.mount
> > 
> > But my patch doesn't take that into account yet.
> 
> This one does now. I left a fairly comprehensive commit log as this
> isn't that easy to explain/understand. If it's too verbose for you I
> can also trim it back a bit.

I have now committed a different fix now, that keeps counting of the
mount points in mount.c, instead of "reaching over" from device.c.

I only gave this light testing, would be cool if you could check if
this fixes things for you.

http://cgit.freedesktop.org/systemd/systemd/commit/?id=fcd8b266edf0df2b85079fcf7b099cd4028740e6

This commit will now collect two sets of devices while going through
/proc/self/mountinfo: the devices of lines that are no longer there,
and the devices of lines that are there. Only for devices in the
former set that are not in the latter we will now propagate an event
to device.c.

Does this make sense?

> +/* If we found a tentative device via mountinfo which is still
> + * referenced by other mounts than the one that just got unmounted, 
> we
> + * must leave it FOUND_MOUNT/tentative; otherwise we'd trigger
> + * unmounting of all other bound mounts. */
> +if (d->found == DEVICE_FOUND_MOUNT && n == 0) {
> +Iterator i;
> +Unit *bound;
> +
> +SET_FOREACH(bound, UNIT(d)->dependencies[UNIT_BOUND_BY], i) {
> +if (bound->type == UNIT_MOUNT &&
> +MOUNT(bound)->state != MOUNT_DEAD && 
> MOUNT(bound)->state != MOUNT_FAILED) {
> +log_unit_debug(UNIT(d), "Still bound by %s 
> (%s), keeping state.",
> +   bound->id, 
> mount_state_to_string(MOUNT(bound)->state));
> +return;
> +}
> +}
> +log_unit_debug(UNIT(d), "Not referenced by any mounts any 
> more, changing to dead.");
> +}
> +
>  previous = d->found;
>  d->found = n;

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v3] device: Fix overzealous unmounting of tentative device mounts

2015-05-19 Thread systemd github import bot 
Patchset imported to github.
Pull request:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel