RE: [PATCH] DAMON dbgfs_mk_context() error handling

[SeongJae Park <sj@kernel.org>]

On Mon, 15 Aug 2022 23:42:10 +0000 SeongJae Park <sj@kernel.org> wrote:

> Hi Badari,
> 
> 
> On Mon, 15 Aug 2022 21:51:04 +0000 "Pulavarty, Badari" <badari.pulavarty@intel.com> wrote:
> 
> > [-- Attachment #1: Type: text/plain, Size: 350 bytes --]
> > 
> > Hi SI,
> > 
> > > Nice finding, thank you!  But, the return value of the debugfs call is intentionally ignored[2].  How about doing the duplicated name check in
> > > dbgfs_mk_context() itself before the debugfs_create_dir() call?
> 
> Please don't unwrap wrapped message.  My monitor is not so wide :'(
> 
> > >
> > > [2] https://lore.kernel.org/linux-mm/20210205155902.31102-1-sjpark@amazon.com/
> > 
> > How about this?
> > 
> > Thanks,
> > Badari
> > 
> > 
> > 
> > [-- Attachment #2: damon-mkcontext-fix.patch --]
> > [-- Type: application/octet-stream, Size: 858 bytes --]
> 
> Could you please don't send a patch as an attached file of the mail but put it
> in the mail body so that we can easily read the patch and comment in line?
> 
> > 
> > damon dbgfs_mk_context() should check to make sure there is no existing 
> > context with the same name. Otherwise, it will cause failure when we 
> > enabling the monitor.
> > 
> > Test case:
> > 
> > echo "off" >  /sys/kernel/debug/damon/monitor_on
> > echo "abc" > /sys/kernel/debug/damon/mk_context
> > echo "abc" > /sys/kernel/debug/damon/mk_context
> > echo <pid> > /sys/kernel/debug/damon/abc/target_ids
> > echo "on" > /sys/kernel/debug/damon/monitor_on  <<< fails 
> > 
> > Signed-off-by: Badari Pulavarty <badari.pulavarty@intel.com>
> > ---
> 
> It would be good to put the changelog of this patch here:
> https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
> 
> > --- a/mm/damon/dbgfs.c	2022-08-15 14:27:38.308806431 -0700
> > +++ b/mm/damon/dbgfs.c	2022-08-15 14:33:31.661163048 -0700
> > @@ -817,6 +817,12 @@
> >  	if (!root)
> >  		return -ENOENT;
> >  
> > +	new_dir = debugfs_lookup(name, root);
> > +	if (new_dir) {
> > +		dput(new_dir);
> > +		return -EEXIST;
> > +	}
> > +
> 
> The change looks ok to me at a glance, but the attached file seems not an
> appropriate patch.  Could you please repost this as a formal patch as suggested
> for a better review?

Also, please add 'Fixes:' tag and Cc stable@vger.kernel.org to the patch.


Thanks,
SJ

> 
> Thanks,
> SJ
> 
> >  	new_dir = debugfs_create_dir(name, root);
> >  	dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;