On Thu, Apr 7, 2022 at 5:35 PM Jan Kara <jack@xxxxxxx> wrote: > > On Tue 29-03-22 10:48:54, Amir Goldstein wrote: > > Create helpers to take and release the group mark_mutex lock. > > > > Define a flag FSNOTIFY_GROUP_NOFS in fsnotify backend operations struct > > that determines if the mark_mutex lock is fs reclaim safe or not. > > If not safe, the nofs lock helpers should be used to take the lock and > > disable direct fs reclaim. > > > > In that case we annotate the mutex with different lockdep class to > > express to lockdep that an allocation of mark of an fs reclaim safe group > > may take the group lock of another "NOFS" group to evict inodes. > > > > For now, converted only the callers in common code and no backend > > defines the NOFS flag. It is intended to be set by fanotify for > > evictable marks support. > > > > Suggested-by: Jan Kara <jack@xxxxxxx> > > Link: https://lore.kernel.org/r/20220321112310.vpr7oxro2xkz5llh@xxxxxxxxxx/ > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > A few design question here: > > 1) Why do you store the FSNOTIFY_GROUP_NOFS flag in ops? Sure, this > particular flag is probably going to be the same per backend type but it > seems a bit strange to have it in ops instead of in the group itself... I followed the pattern of struct file_system_type. I didn't think per-group NOFS made much sense, so it was easier this way. > > 2) Why do we have fsnotify_group_nofs_lock() as well as > fsnotify_group_lock()? We could detect whether we should set nofs based on > group flag anyway. Is that so that callers don't have to bother with passing > around the 'nofs'? Is it too bad? We could also store the old value of > 'nofs' inside the group itself after locking it and then unlock can restore > it without the caller needing to care about anything... Yes because it created unneeded code. storing the local thread state in the group seems odd... Thanks, Amir.
