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
