* [PATCH] ceph: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
@ 2021-12-21 14:36 cgel.zte
2022-01-03 14:41 ` Jeff Layton
0 siblings, 1 reply; 9+ messages in thread
From: cgel.zte @ 2021-12-21 14:36 UTC (permalink / raw)
To: jlayton; +Cc: idryomov, ceph-devel, linux-kernel, Changcheng Deng, Zeal Robot
From: Changcheng Deng <deng.changcheng@zte.com.cn>
Fix the following coccicheck warning:
./fs/ceph/debugfs.c: 390: 0-23: WARNING: congestion_kb_fops should be
defined with DEFINE_DEBUGFS_ATTRIBUTE
Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
debugfs files.
Reported-by: Zeal Robot <zealci@zte.com.cn>
Signed-off-by: Changcheng Deng <deng.changcheng@zte.com.cn>
---
fs/ceph/debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 3cf7c9c1085b..64c4158c17c8 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -387,8 +387,8 @@ static int congestion_kb_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
- congestion_kb_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
+ congestion_kb_set, "%llu\n");
void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2021-12-21 14:36 [PATCH] ceph: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE cgel.zte
@ 2022-01-03 14:41 ` Jeff Layton
2022-01-03 15:03 ` Ilya Dryomov
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2022-01-03 14:41 UTC (permalink / raw)
To: cgel.zte; +Cc: idryomov, ceph-devel, linux-kernel, Changcheng Deng, Zeal Robot
On Tue, 2021-12-21 at 14:36 +0000, cgel.zte@gmail.com wrote:
> From: Changcheng Deng <deng.changcheng@zte.com.cn>
>
> Fix the following coccicheck warning:
> ./fs/ceph/debugfs.c: 390: 0-23: WARNING: congestion_kb_fops should be
> defined with DEFINE_DEBUGFS_ATTRIBUTE
>
> Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
> debugfs files.
>
> Reported-by: Zeal Robot <zealci@zte.com.cn>
> Signed-off-by: Changcheng Deng <deng.changcheng@zte.com.cn>
> ---
> fs/ceph/debugfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 3cf7c9c1085b..64c4158c17c8 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -387,8 +387,8 @@ static int congestion_kb_get(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> - congestion_kb_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> + congestion_kb_set, "%llu\n");
>
>
> void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
Thanks, merged into our testing branch. This should make v5.17.
Cheers,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2022-01-03 14:41 ` Jeff Layton
@ 2022-01-03 15:03 ` Ilya Dryomov
2022-01-03 15:30 ` Jeff Layton
0 siblings, 1 reply; 9+ messages in thread
From: Ilya Dryomov @ 2022-01-03 15:03 UTC (permalink / raw)
To: Jeff Layton; +Cc: cgel.zte, Ceph Development, LKML, Changcheng Deng, Zeal Robot
On Mon, Jan 3, 2022 at 3:41 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2021-12-21 at 14:36 +0000, cgel.zte@gmail.com wrote:
> > From: Changcheng Deng <deng.changcheng@zte.com.cn>
> >
> > Fix the following coccicheck warning:
> > ./fs/ceph/debugfs.c: 390: 0-23: WARNING: congestion_kb_fops should be
> > defined with DEFINE_DEBUGFS_ATTRIBUTE
> >
> > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
> > debugfs files.
> >
> > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > Signed-off-by: Changcheng Deng <deng.changcheng@zte.com.cn>
> > ---
> > fs/ceph/debugfs.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > index 3cf7c9c1085b..64c4158c17c8 100644
> > --- a/fs/ceph/debugfs.c
> > +++ b/fs/ceph/debugfs.c
> > @@ -387,8 +387,8 @@ static int congestion_kb_get(void *data, u64 *val)
> > return 0;
> > }
> >
> > -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > - congestion_kb_set, "%llu\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > + congestion_kb_set, "%llu\n");
> >
> >
> > void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
>
> Thanks, merged into our testing branch. This should make v5.17.
Hi Jeff,
This came up last year, has anything changed?
https://www.spinics.net/lists/ceph-devel/msg50492.html
Thanks,
Ilya
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2022-01-03 15:03 ` Ilya Dryomov
@ 2022-01-03 15:30 ` Jeff Layton
0 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2022-01-03 15:30 UTC (permalink / raw)
To: Ilya Dryomov
Cc: cgel.zte, Ceph Development, LKML, Changcheng Deng, Zeal Robot
On Mon, 2022-01-03 at 16:03 +0100, Ilya Dryomov wrote:
> On Mon, Jan 3, 2022 at 3:41 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Tue, 2021-12-21 at 14:36 +0000, cgel.zte@gmail.com wrote:
> > > From: Changcheng Deng <deng.changcheng@zte.com.cn>
> > >
> > > Fix the following coccicheck warning:
> > > ./fs/ceph/debugfs.c: 390: 0-23: WARNING: congestion_kb_fops should be
> > > defined with DEFINE_DEBUGFS_ATTRIBUTE
> > >
> > > Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE for
> > > debugfs files.
> > >
> > > Reported-by: Zeal Robot <zealci@zte.com.cn>
> > > Signed-off-by: Changcheng Deng <deng.changcheng@zte.com.cn>
> > > ---
> > > fs/ceph/debugfs.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > > index 3cf7c9c1085b..64c4158c17c8 100644
> > > --- a/fs/ceph/debugfs.c
> > > +++ b/fs/ceph/debugfs.c
> > > @@ -387,8 +387,8 @@ static int congestion_kb_get(void *data, u64 *val)
> > > return 0;
> > > }
> > >
> > > -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > > - congestion_kb_set, "%llu\n");
> > > +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > > + congestion_kb_set, "%llu\n");
> > >
> > >
> > > void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
> >
> > Thanks, merged into our testing branch. This should make v5.17.
>
> Hi Jeff,
>
> This came up last year, has anything changed?
>
> https://www.spinics.net/lists/ceph-devel/msg50492.html
>
Good point, that discussion had totally slipped my mind. Dropping this
patch again for now until the rationale is clearer.
Jiapeng, could you clarify why you didn't do the other part of that
conversion?
Thanks,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: Replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2021-02-02 12:45 ` Jeff Layton
@ 2021-02-02 16:01 ` Jeff Layton
0 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2021-02-02 16:01 UTC (permalink / raw)
To: Ilya Dryomov, Jiapeng Chong; +Cc: Ceph Development, LKML
On Tue, 2021-02-02 at 07:45 -0500, Jeff Layton wrote:
> On Tue, 2021-02-02 at 13:07 +0100, Ilya Dryomov wrote:
> > On Mon, Feb 1, 2021 at 8:52 AM Jiapeng Chong
> > <jiapeng.chong@linux.alibaba.com> wrote:
> > >
> > > Fix the following coccicheck warning:
> > >
> > > ./fs/ceph/debugfs.c:347:0-23: WARNING: congestion_kb_fops should be
> > > defined with DEFINE_DEBUGFS_ATTRIBUTE.
> > >
> > > Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> > > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> > > ---
> > > fs/ceph/debugfs.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > > index 66989c8..617327e 100644
> > > --- a/fs/ceph/debugfs.c
> > > +++ b/fs/ceph/debugfs.c
> > > @@ -344,8 +344,8 @@ static int congestion_kb_get(void *data, u64 *val)
> > > return 0;
> > > }
> > >
> > > -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > > - congestion_kb_set, "%llu\n");
> > > +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > > + congestion_kb_set, "%llu\n");
> > >
> > >
> > > void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
> >
> > Hi Jiapeng,
> >
> > What is the benefit of this conversion?
> >
> > From a quick look, with DEFINE_DEBUGFS_ATTRIBUTE writeback_congestion_kb
> > file would no longer be seekable. It may not matter much, but something
> > that should have been mentioned.
> >
> > Futher, debugfs_create_file() creates a full proxy for fops, protecting
> > against removal races. DEFINE_DEBUGFS_ATTRIBUTE adds its own protection
> > but just for ->read() and ->write(). I don't think we need both.
> >
>
>
> The coccinelle script clarifies some of this. See the commit log here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5103068eaca290f890a30aae70085fac44cecaf6
>
> That said, it also mentions that the file should be converted to use
> debugfs_create_file_unsafe now as well, and that wasn't done in this
> patch. Jiapeng, was that intentional? If so, why?
>
For now, I've dropped this patch until the situation with
debugfs_create_file_unsafe is clear.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: Replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2021-02-02 12:07 ` Ilya Dryomov
@ 2021-02-02 12:45 ` Jeff Layton
2021-02-02 16:01 ` Jeff Layton
0 siblings, 1 reply; 9+ messages in thread
From: Jeff Layton @ 2021-02-02 12:45 UTC (permalink / raw)
To: Ilya Dryomov, Jiapeng Chong; +Cc: Ceph Development, LKML
On Tue, 2021-02-02 at 13:07 +0100, Ilya Dryomov wrote:
> On Mon, Feb 1, 2021 at 8:52 AM Jiapeng Chong
> <jiapeng.chong@linux.alibaba.com> wrote:
> >
> > Fix the following coccicheck warning:
> >
> > ./fs/ceph/debugfs.c:347:0-23: WARNING: congestion_kb_fops should be
> > defined with DEFINE_DEBUGFS_ATTRIBUTE.
> >
> > Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> > ---
> > fs/ceph/debugfs.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> > index 66989c8..617327e 100644
> > --- a/fs/ceph/debugfs.c
> > +++ b/fs/ceph/debugfs.c
> > @@ -344,8 +344,8 @@ static int congestion_kb_get(void *data, u64 *val)
> > return 0;
> > }
> >
> > -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > - congestion_kb_set, "%llu\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> > + congestion_kb_set, "%llu\n");
> >
> >
> > void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
>
> Hi Jiapeng,
>
> What is the benefit of this conversion?
>
> From a quick look, with DEFINE_DEBUGFS_ATTRIBUTE writeback_congestion_kb
> file would no longer be seekable. It may not matter much, but something
> that should have been mentioned.
>
> Futher, debugfs_create_file() creates a full proxy for fops, protecting
> against removal races. DEFINE_DEBUGFS_ATTRIBUTE adds its own protection
> but just for ->read() and ->write(). I don't think we need both.
>
The coccinelle script clarifies some of this. See the commit log here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5103068eaca290f890a30aae70085fac44cecaf6
That said, it also mentions that the file should be converted to use
debugfs_create_file_unsafe now as well, and that wasn't done in this
patch. Jiapeng, was that intentional? If so, why?
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: Replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2021-02-01 7:52 [PATCH] ceph: Replace " Jiapeng Chong
2021-02-01 19:01 ` Jeff Layton
@ 2021-02-02 12:07 ` Ilya Dryomov
2021-02-02 12:45 ` Jeff Layton
1 sibling, 1 reply; 9+ messages in thread
From: Ilya Dryomov @ 2021-02-02 12:07 UTC (permalink / raw)
To: Jiapeng Chong; +Cc: Jeff Layton, Ceph Development, LKML
On Mon, Feb 1, 2021 at 8:52 AM Jiapeng Chong
<jiapeng.chong@linux.alibaba.com> wrote:
>
> Fix the following coccicheck warning:
>
> ./fs/ceph/debugfs.c:347:0-23: WARNING: congestion_kb_fops should be
> defined with DEFINE_DEBUGFS_ATTRIBUTE.
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
> fs/ceph/debugfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 66989c8..617327e 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -344,8 +344,8 @@ static int congestion_kb_get(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> - congestion_kb_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> + congestion_kb_set, "%llu\n");
>
>
> void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
Hi Jiapeng,
What is the benefit of this conversion?
From a quick look, with DEFINE_DEBUGFS_ATTRIBUTE writeback_congestion_kb
file would no longer be seekable. It may not matter much, but something
that should have been mentioned.
Futher, debugfs_create_file() creates a full proxy for fops, protecting
against removal races. DEFINE_DEBUGFS_ATTRIBUTE adds its own protection
but just for ->read() and ->write(). I don't think we need both.
Thanks,
Ilya
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ceph: Replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
2021-02-01 7:52 [PATCH] ceph: Replace " Jiapeng Chong
@ 2021-02-01 19:01 ` Jeff Layton
2021-02-02 12:07 ` Ilya Dryomov
1 sibling, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2021-02-01 19:01 UTC (permalink / raw)
To: Jiapeng Chong; +Cc: idryomov, ceph-devel, linux-kernel
On Mon, 2021-02-01 at 15:52 +0800, Jiapeng Chong wrote:
> Fix the following coccicheck warning:
>
> ./fs/ceph/debugfs.c:347:0-23: WARNING: congestion_kb_fops should be
> defined with DEFINE_DEBUGFS_ATTRIBUTE.
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
> fs/ceph/debugfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 66989c8..617327e 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -344,8 +344,8 @@ static int congestion_kb_get(void *data, u64 *val)
> return 0;
> }
>
>
> -DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> - congestion_kb_set, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
> + congestion_kb_set, "%llu\n");
>
>
>
>
> void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
Looks reasonable. Merged into ceph-client/testing branch.
Thanks!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] ceph: Replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
@ 2021-02-01 7:52 Jiapeng Chong
2021-02-01 19:01 ` Jeff Layton
2021-02-02 12:07 ` Ilya Dryomov
0 siblings, 2 replies; 9+ messages in thread
From: Jiapeng Chong @ 2021-02-01 7:52 UTC (permalink / raw)
To: jlayton; +Cc: idryomov, ceph-devel, linux-kernel, Jiapeng Chong
Fix the following coccicheck warning:
./fs/ceph/debugfs.c:347:0-23: WARNING: congestion_kb_fops should be
defined with DEFINE_DEBUGFS_ATTRIBUTE.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
fs/ceph/debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 66989c8..617327e 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -344,8 +344,8 @@ static int congestion_kb_get(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
- congestion_kb_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(congestion_kb_fops, congestion_kb_get,
+ congestion_kb_set, "%llu\n");
void ceph_fs_debugfs_cleanup(struct ceph_fs_client *fsc)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-01-03 15:30 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-21 14:36 [PATCH] ceph: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE cgel.zte
2022-01-03 14:41 ` Jeff Layton
2022-01-03 15:03 ` Ilya Dryomov
2022-01-03 15:30 ` Jeff Layton
-- strict thread matches above, loose matches on Subject: below --
2021-02-01 7:52 [PATCH] ceph: Replace " Jiapeng Chong
2021-02-01 19:01 ` Jeff Layton
2021-02-02 12:07 ` Ilya Dryomov
2021-02-02 12:45 ` Jeff Layton
2021-02-02 16:01 ` Jeff Layton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.