On Mon, Jul 27, 2020 at 10:44 AM Jan Kara <jack@xxxxxxx> wrote: > > On Sun 26-07-20 18:20:26, Amir Goldstein wrote: > > On Thu, Jul 9, 2020 at 8:56 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > > > > Otherwise the patch looks good. One observation though: The (mask & > > > > > > FS_MODIFY) check means that all vfs_write() calls end up going through the > > > > > > "slower" path iterating all mark types and checking whether there are marks > > > > > > anyway. That could be relatively simply optimized using a hidden mask flag > > > > > > like FS_ALWAYS_RECEIVE_MODIFY which would be set when there's some mark > > > > > > needing special handling of FS_MODIFY... Not sure if we care enough at this > > > > > > point... > > > > > > > > > > Yeh that sounds low hanging. > > > > > Actually, I Don't think we need to define a flag for that. > > > > > __fsnotify_recalc_mask() can add FS_MODIFY to the object's mask if needed. > > > > > > > > Yes, that would be even more elegant. > > > > > > > > > I will take a look at that as part of FS_PRE_MODIFY work. > > > > > But in general, we should fight the urge to optimize theoretic > > > > > performance issues... > > > > > > > > Agreed. I just suspect this may bring measurable benefit for hackbench pipe > > > > or tiny tmpfs writes after seeing Mel's results. But as I wrote this is a > > > > separate idea and without some numbers confirming my suspicion I don't > > > > think the complication is worth it so I don't want you to burn time on this > > > > unless you're really interested :). > > > > > > > > > > You know me ;-) > > > FS_MODIFY optimization pushed to fsnotify_pre_modify branch. > > > Only tested that LTP tests pass. > > > > > > Note that this is only expected to improve performance in case there *are* > > > marks, but not marks with ignore mask, because there is an earlier > > > optimization in fsnotify() for the no marks case. > > > > > > > Hi Mel, > > > > After following up on Jan's suggestion above, I realized there is another > > low hanging optimization we can make. > > > > As you may remember, one of the solutions we considered was to exclude > > special or internal sb's from notifications based on some SB flag, but making > > assumptions about which sb are expected to provide notifications turned out > > to be a risky game. > > > > We can however, keep a counter on sb to *know* there are no watches > > on any object in this sb, so the test: > > > > if (!sb->s_fsnotify_marks && > > (!mnt || !mnt->mnt_fsnotify_marks) && > > (!inode || !inode->i_fsnotify_marks)) > > return 0; > > > > Which is not so nice for inlining, can be summarized as: > > > > if (atomic_long_read(&inode->i_sb->s_fsnotify_obj_refs) == 0) > > return 0; > > > > Which is nicer for inlining. > > That's a nice idea. I was just wondering why do you account only inode > references in the superblock. Because if there's only say mount watch, > s_fsnotify_obj_refs will be 0 and you will wrongly skip reporting. Or am I > misunderstanding something? I'd rather have counter like > sb->s_fsnotify_connectors, that will account all connectors related to the > superblock - i.e., connectors attached to the superblock, mounts referring > to the superblock, or inodes referring to the superblock... > Yeh, that is what I did. Those two commits change the former s_fsnotify_inode_refs to s_fsnotify_obj_refs which counts objects (inodes/mounts/sb) pointed to be connectors. I agree that s_fsnotify_connectors may be a better choice of name ;-) de1255f8a64c fsnotify: count all objects with attached connectors 5e6c3af6e2df fsnotify: count s_fsnotify_inode_refs for attached connectors Thanks, Amir.
