Re: [PATCH 13/17] watch_queue: Implement mount topology and attribute change notifications [ver #5]
From: Ian Kent
Date: Tue Aug 04 2020 - 21:54:01 EST
On Tue, 2020-08-04 at 15:19 +0200, Miklos Szeredi wrote:
> On Tue, Aug 4, 2020 at 1:39 PM Ian Kent <raven@xxxxxxxxxx> wrote:
> > On Mon, 2020-08-03 at 11:29 +0200, Miklos Szeredi wrote:
> > > On Thu, Jul 23, 2020 at 12:48 PM David Howells <
> > > dhowells@xxxxxxxxxx>
> > > wrote:
> > >
> > > > > > __u32 topology_changes;
> > > > > > __u32 attr_changes;
> > > > > > __u32 aux_topology_changes;
> > > > >
> > > > > Being 32bit this introduces wraparound effects. Is that
> > > > > really
> > > > > worth it?
> > > >
> > > > You'd have to make 2 billion changes without whoever's
> > > > monitoring
> > > > getting a
> > > > chance to update their counters. But maybe it's not worth it
> > > > putting them
> > > > here. If you'd prefer, I can make the counters all 64-bit and
> > > > just
> > > > retrieve
> > > > them with fsinfo().
> > >
> > > Yes, I think that would be preferable.
> > I think this is the source of the recommendation for removing the
> > change counters from the notification message, correct?
> > While it looks like I may not need those counters for systemd
> > message
> > buffer overflow handling myself I think removing them from the
> > notification message isn't a sensible thing to do.
> > If you need to detect missing messages, perhaps due to message
> > buffer
> > overflow, then you need change counters that are relevant to the
> > notification message itself. That's so the next time you get a
> > message
> > for that object you can be sure that change counter comparisons you
> > you make relate to object notifications you have processed.
> I don't quite get it. Change notification is just that: a
> notification. You need to know what object that notification
> to, to be able to retrieve the up to date attributes of said object.
> What happens if you get a change counter N in the notification
> message, then get a change counter N + 1 in the attribute retrieval?
> You know that another change happened, and you haven't yet processed
> the notification yet. So when the notification with N + 1 comes in,
> you can optimize away the attribute retrieve.
> Nice optimization, but it's optimizing a race condition, and I don't
> think that's warranted. I don't see any other use for the change
> counter in the notification message.
> > Yes, I know it isn't quite that simple, but tallying up what you
> > have
> > processed in the current batch of messages (or in multiple batches
> > of
> > messages if more than one read has been possible) to perform the
> > check
> > is a user space responsibility. And it simply can't be done if the
> > counters consistency is in question which it would be if you need
> > to
> > perform another system call to get it.
> > It's way more useful to have these in the notification than
> > obtainable
> > via fsinfo() IMHO.
> What is it useful for?
Only to verify that you have seen all the notifications.
If you have to grab that info with a separate call then the count
isn't necessarily consistent because other notifications can occur
while you grab it.
My per-object rant isn't quite right, what's needed is a consistent
way to verify you have seen everything you were supposed to.
I think your point is that if you grab the info in another call and
it doesn't match you need to refresh and that's fine but I think it's
better to be able to verify you have got everything that was sent as
you go and avoid the need for the refresh more often.
> If the notification itself would contain the list of updated
> attributes and their new values, then yes, this would make sense. If
> the notification just tells us that the object was modified, but not
> the modifications themselves, then I don't see how the change counter
> in itself could add any information (other than optimizing the race
> condition above).
> > > > > > n->watch.info & NOTIFY_MOUNT_IS_RECURSIVE if true
> > > > > > indicates that
> > > > > > the notifcation was generated by an event (eg.
> > > > > > SETATTR)
> > > > > > that was
> > > > > > applied recursively. The notification is only
> > > > > > generated for the
> > > > > > object that initially triggered it.
> > > > >
> > > > > Unused in this patchset. Please don't add things to the API
> > > > > which are not
> > > > > used.
> > > >
> > > > Christian Brauner has patches for mount_setattr() that will
> > > > need to
> > > > use this.
> > >
> > > Fine, then that patch can add the flag.
> > >
> > > Thanks,
> > > Miklos