On Fri 08-04-22 11:38:59, Amir Goldstein wrote: > On Thu, Apr 7, 2022 at 5:53 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > 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. I see. Yes, if they are unmutable, the having them in ops is probably fine. But I'd rename them to say 'features' to better explain they are unmutable. > > > 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... > > > > I followed your suggestions and the result looks much better. > Pushed result to fan_evictable branch. Thanks! Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
