Re: [PATCH 13/17] watch_queue: Implement mount topology and attribute change notifications [ver #5]

[Ian Kent]

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
> relates
> 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).
> 
> Thanks,
> Miklos
> 
> Thanks,
> 
> 
> 
> > > > > >         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