On Mon 11-04-22 15:57:30, Amir Goldstein wrote: > On Mon, Apr 11, 2022 at 2:47 PM Jan Kara <jack@xxxxxxx> wrote: > > > > On Tue 29-03-22 10:49:01, Amir Goldstein wrote: > > > When an inode mark is created with flag FAN_MARK_EVICTABLE, it will not > > > pin the marked inode to inode cache, so when inode is evicted from cache > > > due to memory pressure, the mark will be lost. > > > > > > When an inode mark with flag FAN_MARK_EVICATBLE is updated without using > > > this flag, the marked inode is pinned to inode cache. > > > > > > When an inode mark is updated with flag FAN_MARK_EVICTABLE but an > > > existing mark already has the inode pinned, the mark update fails with > > > error EEXIST. > > > > I was thinking about this. FAN_MARK_EVICTABLE is effectively a hint to the > > kernel - you can drop this if you wish. So does it make sense to return > > error when we cannot follow the hint? Doesn't this just add unnecessary > > work (determining whether the mark should be evictable or not) to the > > userspace application using FAN_MARK_EVICTABLE? > > I do not fully agree about your definition of "hint to the kernel". > Yes, for a single inode it may be a hint, but for a million inodes it is pretty > much a directive that setting a very large number of evictable marks > CANNOT be used to choke the system. > > It's true that the application should be able to avoid shooting its own > foot and we do not need to be the ones providing this protection, but > I rather prefer to keep the API more strict and safe than being sorry later. > After all, I don't think this complicates the implementation nor documentation > too much. Is it? see: > > https://github.com/amir73il/man-pages/commit/b52eb7d1a8478cbd1456f4d9463902bbc4e80f0d No, it is not complicating things too much and you're probably right that having things stricter now may pay off in the future. I was just thinking that app adding ignore mark now needs to remember whether it has already added something else for the inode or not to know whether it can use FAN_MARK_EVICTABLE. Which seemed like unnecessary complication. > > I'd also note that FSNOTIFY_MARK_FLAG_NO_IREF needs to be stored only > > because of this error checking behavior. Otherwise it would be enough to > > have a flag on the connector (whether it holds iref or not) and > > fsnotify_add_mark() would update the connector as needed given the added > > I am not sure I agree to that. > Maybe I am missing something, but the way fsnotify_recalc_mask() works now > is by checking if there is any mark without FSNOTIFY_MARK_FLAG_NO_IREF > attached to the object, so fsnotify_put_mark() knows to drop the inode when the > last non-evictable mark is removed. Right, I was confused and somehow thought that once connector has iref, it will drop it only when the mark list gets empty which is obviously not true. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
