Message ID | 20200708111156.24659-8-amir73il@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3,01/20] fsnotify: Rearrange fast path to minimise overhead when there is no watcher | expand |
Hi Amir, I love your patch! Perhaps something to improve: [auto build test WARNING on ext3/fsnotify] [also build test WARNING on nfsd/nfsd-next driver-core/driver-core-testing linus/master v5.8-rc4 next-20200707] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Amir-Goldstein/fsnotify-Rearrange-fast-path-to-minimise-overhead-when-there-is-no-watcher/20200708-191525 base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify config: i386-randconfig-r015-20200708 (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> fs/notify/fanotify/fanotify.c:347:24: warning: no previous prototype for 'fanotify_alloc_path_event' [-Wmissing-prototypes] 347 | struct fanotify_event *fanotify_alloc_path_event(const struct path *path, | ^~~~~~~~~~~~~~~~~~~~~~~~~ >> fs/notify/fanotify/fanotify.c:363:24: warning: no previous prototype for 'fanotify_alloc_perm_event' [-Wmissing-prototypes] 363 | struct fanotify_event *fanotify_alloc_perm_event(const struct path *path, | ^~~~~~~~~~~~~~~~~~~~~~~~~ >> fs/notify/fanotify/fanotify.c:381:24: warning: no previous prototype for 'fanotify_alloc_fid_event' [-Wmissing-prototypes] 381 | struct fanotify_event *fanotify_alloc_fid_event(struct inode *id, | ^~~~~~~~~~~~~~~~~~~~~~~~ >> fs/notify/fanotify/fanotify.c:398:24: warning: no previous prototype for 'fanotify_alloc_name_event' [-Wmissing-prototypes] 398 | struct fanotify_event *fanotify_alloc_name_event(struct inode *id, | ^~~~~~~~~~~~~~~~~~~~~~~~~ vim +/fanotify_alloc_path_event +347 fs/notify/fanotify/fanotify.c 346 > 347 struct fanotify_event *fanotify_alloc_path_event(const struct path *path, 348 gfp_t gfp) 349 { 350 struct fanotify_path_event *pevent; 351 352 pevent = kmem_cache_alloc(fanotify_path_event_cachep, gfp); 353 if (!pevent) 354 return NULL; 355 356 pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH; 357 pevent->path = *path; 358 path_get(path); 359 360 return &pevent->fae; 361 } 362 > 363 struct fanotify_event *fanotify_alloc_perm_event(const struct path *path, 364 gfp_t gfp) 365 { 366 struct fanotify_perm_event *pevent; 367 368 pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp); 369 if (!pevent) 370 return NULL; 371 372 pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM; 373 pevent->response = 0; 374 pevent->state = FAN_EVENT_INIT; 375 pevent->path = *path; 376 path_get(path); 377 378 return &pevent->fae; 379 } 380 > 381 struct fanotify_event *fanotify_alloc_fid_event(struct inode *id, 382 __kernel_fsid_t *fsid, 383 gfp_t gfp) 384 { 385 struct fanotify_fid_event *ffe; 386 387 ffe = kmem_cache_alloc(fanotify_fid_event_cachep, gfp); 388 if (!ffe) 389 return NULL; 390 391 ffe->fae.type = FANOTIFY_EVENT_TYPE_FID; 392 ffe->fsid = *fsid; 393 fanotify_encode_fh(&ffe->object_fh, id, gfp); 394 395 return &ffe->fae; 396 } 397 > 398 struct fanotify_event *fanotify_alloc_name_event(struct inode *id, 399 __kernel_fsid_t *fsid, 400 const struct qstr *file_name, 401 gfp_t gfp) 402 { 403 struct fanotify_name_event *fne; 404 405 fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp); 406 if (!fne) 407 return NULL; 408 409 fne->fae.type = FANOTIFY_EVENT_TYPE_FID_NAME; 410 fne->fsid = *fsid; 411 fanotify_encode_fh(&fne->dir_fh, id, gfp); 412 fne->name_len = file_name->len; 413 strcpy(fne->name, file_name->name); 414 415 return &fne->fae; 416 } 417 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Jul 9, 2020 at 10:33 AM kernel test robot <lkp@intel.com> wrote: > > Hi Amir, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on ext3/fsnotify] > [also build test WARNING on nfsd/nfsd-next driver-core/driver-core-testing linus/master v5.8-rc4 next-20200708] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Amir-Goldstein/fsnotify-Rearrange-fast-path-to-minimise-overhead-when-there-is-no-watcher/20200708-191525 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify > config: x86_64-allyesconfig (attached as .config) > compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install x86_64 cross compiling tool for clang build > # apt-get install binutils-x86-64-linux-gnu > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > >> fs/notify/fanotify/fanotify.c:347:24: warning: no previous prototype for function 'fanotify_alloc_path_event' [-Wmissing-prototypes] > struct fanotify_event *fanotify_alloc_path_event(const struct path *path, > ^ > fs/notify/fanotify/fanotify.c:347:1: note: declare 'static' if the function is not intended to be used outside of this translation unit > struct fanotify_event *fanotify_alloc_path_event(const struct path *path, > ^ > static > >> fs/notify/fanotify/fanotify.c:363:24: warning: no previous prototype for function 'fanotify_alloc_perm_event' [-Wmissing-prototypes] > struct fanotify_event *fanotify_alloc_perm_event(const struct path *path, > ^ > fs/notify/fanotify/fanotify.c:363:1: note: declare 'static' if the function is not intended to be used outside of this translation unit > struct fanotify_event *fanotify_alloc_perm_event(const struct path *path, > ^ > static > >> fs/notify/fanotify/fanotify.c:381:24: warning: no previous prototype for function 'fanotify_alloc_fid_event' [-Wmissing-prototypes] > struct fanotify_event *fanotify_alloc_fid_event(struct inode *id, > ^ > fs/notify/fanotify/fanotify.c:381:1: note: declare 'static' if the function is not intended to be used outside of this translation unit > struct fanotify_event *fanotify_alloc_fid_event(struct inode *id, > ^ > static > >> fs/notify/fanotify/fanotify.c:398:24: warning: no previous prototype for function 'fanotify_alloc_name_event' [-Wmissing-prototypes] > struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > ^ > fs/notify/fanotify/fanotify.c:398:1: note: declare 'static' if the function is not intended to be used outside of this translation unit > struct fanotify_event *fanotify_alloc_name_event(struct inode *id, > ^ > static > 4 warnings generated. Jan, I add 'static' rebased and pushed to fanotify_prep branch Rebase had minor conflict in following patch (pass dir argument ...) Also rebased and pushed fanotify_name_fid Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 921ff05e1877..c4ada3501014 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -344,6 +344,77 @@ static struct inode *fanotify_fid_inode(struct inode *to_tell, u32 event_mask, return fsnotify_data_inode(data, data_type); } +struct fanotify_event *fanotify_alloc_path_event(const struct path *path, + gfp_t gfp) +{ + struct fanotify_path_event *pevent; + + pevent = kmem_cache_alloc(fanotify_path_event_cachep, gfp); + if (!pevent) + return NULL; + + pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH; + pevent->path = *path; + path_get(path); + + return &pevent->fae; +} + +struct fanotify_event *fanotify_alloc_perm_event(const struct path *path, + gfp_t gfp) +{ + struct fanotify_perm_event *pevent; + + pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp); + if (!pevent) + return NULL; + + pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM; + pevent->response = 0; + pevent->state = FAN_EVENT_INIT; + pevent->path = *path; + path_get(path); + + return &pevent->fae; +} + +struct fanotify_event *fanotify_alloc_fid_event(struct inode *id, + __kernel_fsid_t *fsid, + gfp_t gfp) +{ + struct fanotify_fid_event *ffe; + + ffe = kmem_cache_alloc(fanotify_fid_event_cachep, gfp); + if (!ffe) + return NULL; + + ffe->fae.type = FANOTIFY_EVENT_TYPE_FID; + ffe->fsid = *fsid; + fanotify_encode_fh(&ffe->object_fh, id, gfp); + + return &ffe->fae; +} + +struct fanotify_event *fanotify_alloc_name_event(struct inode *id, + __kernel_fsid_t *fsid, + const struct qstr *file_name, + gfp_t gfp) +{ + struct fanotify_name_event *fne; + + fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp); + if (!fne) + return NULL; + + fne->fae.type = FANOTIFY_EVENT_TYPE_FID_NAME; + fne->fsid = *fsid; + fanotify_encode_fh(&fne->dir_fh, id, gfp); + fne->name_len = file_name->len; + strcpy(fne->name, file_name->name); + + return &fne->fae; +} + static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, struct inode *inode, u32 mask, const void *data, int data_type, @@ -351,8 +422,6 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, __kernel_fsid_t *fsid) { struct fanotify_event *event = NULL; - struct fanotify_fid_event *ffe = NULL; - struct fanotify_name_event *fne = NULL; gfp_t gfp = GFP_KERNEL_ACCOUNT; struct inode *id = fanotify_fid_inode(inode, mask, data, data_type); const struct path *path = fsnotify_data_path(data, data_type); @@ -372,55 +441,23 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, memalloc_use_memcg(group->memcg); if (fanotify_is_perm_event(mask)) { - struct fanotify_perm_event *pevent; - - pevent = kmem_cache_alloc(fanotify_perm_event_cachep, gfp); - if (!pevent) - goto out; - - event = &pevent->fae; - event->type = FANOTIFY_EVENT_TYPE_PATH_PERM; - pevent->response = 0; - pevent->state = FAN_EVENT_INIT; - goto init; - } - - /* - * For FAN_DIR_MODIFY event, we report the fid of the directory and - * the name of the modified entry. - * Allocate an fanotify_name_event struct and copy the name. - */ - if (mask & FAN_DIR_MODIFY && !(WARN_ON_ONCE(!file_name))) { - fne = kmalloc(sizeof(*fne) + file_name->len + 1, gfp); - if (!fne) - goto out; - - event = &fne->fae; - event->type = FANOTIFY_EVENT_TYPE_FID_NAME; - fne->name_len = file_name->len; - strcpy(fne->name, file_name->name); - goto init; - } - - if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { - ffe = kmem_cache_alloc(fanotify_fid_event_cachep, gfp); - if (!ffe) - goto out; - - event = &ffe->fae; - event->type = FANOTIFY_EVENT_TYPE_FID; + event = fanotify_alloc_perm_event(path, gfp); + } else if (mask & FAN_DIR_MODIFY && !(WARN_ON_ONCE(!file_name))) { + /* + * For FAN_DIR_MODIFY event, we report the fid of the directory + * and the name of the modified entry. + * Allocate an fanotify_name_event struct and copy the name. + */ + event = fanotify_alloc_name_event(id, fsid, file_name, gfp); + } else if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { + event = fanotify_alloc_fid_event(id, fsid, gfp); } else { - struct fanotify_path_event *pevent; - - pevent = kmem_cache_alloc(fanotify_path_event_cachep, gfp); - if (!pevent) - goto out; - - event = &pevent->fae; - event->type = FANOTIFY_EVENT_TYPE_PATH; + event = fanotify_alloc_path_event(path, gfp); } -init: + if (!event) + goto out; + /* * Use the victim inode instead of the watching inode as the id for * event queue, so event reported on parent is merged with event @@ -432,19 +469,6 @@ static struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group, else event->pid = get_pid(task_tgid(current)); - if (fsid && fanotify_event_fsid(event)) - *fanotify_event_fsid(event) = *fsid; - - if (fanotify_event_object_fh(event)) - fanotify_encode_fh(fanotify_event_object_fh(event), id, gfp); - - if (fanotify_event_dir_fh(event)) - fanotify_encode_fh(fanotify_event_dir_fh(event), id, gfp); - - if (fanotify_event_has_path(event)) { - *fanotify_event_path(event) = *path; - path_get(path); - } out: memalloc_unuse_memcg(); return event;
Break up fanotify_alloc_event() into helpers by event struct type. Suggested-by: Jan Kara <jack@suse.cz> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.c | 146 ++++++++++++++++++++-------------- 1 file changed, 85 insertions(+), 61 deletions(-)