* [PATCH -mm 0/2] nilfs2: reduce build warnings with "make C=1"
@ 2024-04-30 8:00 Ryusuke Konishi
2024-04-30 8:00 ` [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header Ryusuke Konishi
2024-04-30 8:00 ` [PATCH -mm 2/2] nilfs2: make superblock data array index computation sparse friendly Ryusuke Konishi
0 siblings, 2 replies; 10+ messages in thread
From: Ryusuke Konishi @ 2024-04-30 8:00 UTC (permalink / raw
To: Andrew Morton; +Cc: linux-nilfs, linux-kernel, Bart Van Assche, Jens Axboe
Hi Andrew,
Please add these to the queue for the next merge window.
These two are cleanups that eliminate the warnings output by sparse
(not including common one related to refcount_dec_and_lock).
Neither is a bugfix.
Thanks,
Ryusuke Konishi
Ryusuke Konishi (2):
nilfs2: use integer type instead of enum req_op for event tracing
header
nilfs2: make superblock data array index computation sparse friendly
fs/nilfs2/mdt.c | 2 +-
fs/nilfs2/the_nilfs.c | 20 ++++++++++++++++++--
include/trace/events/nilfs2.h | 4 ++--
3 files changed, 21 insertions(+), 5 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-04-30 8:00 [PATCH -mm 0/2] nilfs2: reduce build warnings with "make C=1" Ryusuke Konishi
@ 2024-04-30 8:00 ` Ryusuke Konishi
2024-05-01 14:42 ` Bart Van Assche
2024-04-30 8:00 ` [PATCH -mm 2/2] nilfs2: make superblock data array index computation sparse friendly Ryusuke Konishi
1 sibling, 1 reply; 10+ messages in thread
From: Ryusuke Konishi @ 2024-04-30 8:00 UTC (permalink / raw
To: Andrew Morton; +Cc: linux-nilfs, linux-kernel, Bart Van Assche, Jens Axboe
The sparse check with "make C=1" outputs warnings regarding references
to the header file "include/trace/events/nilfs2.h" for event tracing:
fs/nilfs2/segment.c: note: in included file (through
include/trace/trace_events.h, include/trace/define_trace.h,
include/trace/events/nilfs2.h):
./include/trace/events/nilfs2.h:191:1: warning: cast to restricted
blk_opf_t
./include/trace/events/nilfs2.h:191:1: warning: restricted blk_opf_t
degrades to integer
./include/trace/events/nilfs2.h:191:1: warning: restricted blk_opf_t
degrades to integer
Fix this issue by reverting the type of the parameter related to the
bio operation mode in the event tracing definition from "enum req_op"
to "int".
In order to prevent sparse warnings on the caller side (where
trace_nilfs2_mdt_submit_block() is used), also add a typecast to an
argument passed to the tracepoint.
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401092241.I4mm9OWl-lkp@intel.com/
Fixes: ed4512590bd5 ("fs/nilfs2: Use the enum req_op and blk_opf_t types")
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Jens Axboe <axboe@kernel.dk>
---
fs/nilfs2/mdt.c | 2 +-
include/trace/events/nilfs2.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
index 4f792a0ad0f0..323eb8442e0a 100644
--- a/fs/nilfs2/mdt.c
+++ b/fs/nilfs2/mdt.c
@@ -152,7 +152,7 @@ nilfs_mdt_submit_block(struct inode *inode, unsigned long blkoff, blk_opf_t opf,
ret = 0;
trace_nilfs2_mdt_submit_block(inode, inode->i_ino, blkoff,
- opf & REQ_OP_MASK);
+ (__force int)(opf & REQ_OP_MASK));
out:
get_bh(bh);
*out_bh = bh;
diff --git a/include/trace/events/nilfs2.h b/include/trace/events/nilfs2.h
index 8efc6236f57c..84ee31fc04cc 100644
--- a/include/trace/events/nilfs2.h
+++ b/include/trace/events/nilfs2.h
@@ -192,7 +192,7 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
TP_PROTO(struct inode *inode,
unsigned long ino,
unsigned long blkoff,
- enum req_op mode),
+ int mode),
TP_ARGS(inode, ino, blkoff, mode),
@@ -200,7 +200,7 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
__field(struct inode *, inode)
__field(unsigned long, ino)
__field(unsigned long, blkoff)
- __field(enum req_op, mode)
+ __field(int, mode)
),
TP_fast_assign(
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH -mm 2/2] nilfs2: make superblock data array index computation sparse friendly
2024-04-30 8:00 [PATCH -mm 0/2] nilfs2: reduce build warnings with "make C=1" Ryusuke Konishi
2024-04-30 8:00 ` [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header Ryusuke Konishi
@ 2024-04-30 8:00 ` Ryusuke Konishi
1 sibling, 0 replies; 10+ messages in thread
From: Ryusuke Konishi @ 2024-04-30 8:00 UTC (permalink / raw
To: Andrew Morton; +Cc: linux-nilfs, linux-kernel, Bart Van Assche, Jens Axboe
Upon running sparse, "warning: dubious: x & !y" is output at an array
index calculation within nilfs_load_super_block().
The calculation is not wrong, but to eliminate the sparse warning,
replace it with an equivalent calculation.
Also, add a comment to make it easier to understand what the unintuitive
array index calculation is doing and whether it's correct.
Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Fixes: e339ad31f599 ("nilfs2: introduce secondary super block")
---
fs/nilfs2/the_nilfs.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index db322068678f..f41d7b6d432c 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -592,7 +592,7 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
struct nilfs_super_block **sbp = nilfs->ns_sbp;
struct buffer_head **sbh = nilfs->ns_sbh;
u64 sb2off, devsize = bdev_nr_bytes(nilfs->ns_bdev);
- int valid[2], swp = 0;
+ int valid[2], swp = 0, older;
if (devsize < NILFS_SEG_MIN_BLOCKS * NILFS_MIN_BLOCK_SIZE + 4096) {
nilfs_err(sb, "device size too small");
@@ -648,9 +648,25 @@ static int nilfs_load_super_block(struct the_nilfs *nilfs,
if (swp)
nilfs_swap_super_block(nilfs);
+ /*
+ * Calculate the array index of the older superblock data.
+ * If one has been dropped, set index 0 pointing to the remaining one,
+ * otherwise set index 1 pointing to the old one (including if both
+ * are the same).
+ *
+ * Divided case valid[0] valid[1] swp -> older
+ * -------------------------------------------------------------
+ * Both SBs are invalid 0 0 N/A (Error)
+ * SB1 is invalid 0 1 1 0
+ * SB2 is invalid 1 0 0 0
+ * SB2 is newer 1 1 1 0
+ * SB2 is older or the same 1 1 0 1
+ */
+ older = valid[1] ^ swp;
+
nilfs->ns_sbwcount = 0;
nilfs->ns_sbwtime = le64_to_cpu(sbp[0]->s_wtime);
- nilfs->ns_prot_seq = le64_to_cpu(sbp[valid[1] & !swp]->s_last_seq);
+ nilfs->ns_prot_seq = le64_to_cpu(sbp[older]->s_last_seq);
*sbpp = sbp[0];
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-04-30 8:00 ` [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header Ryusuke Konishi
@ 2024-05-01 14:42 ` Bart Van Assche
2024-05-01 15:30 ` Ryusuke Konishi
0 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2024-05-01 14:42 UTC (permalink / raw
To: Ryusuke Konishi, Andrew Morton; +Cc: linux-nilfs, linux-kernel, Jens Axboe
On 4/30/24 10:00, Ryusuke Konishi wrote:
> trace_nilfs2_mdt_submit_block(inode, inode->i_ino, blkoff,
> - opf & REQ_OP_MASK);
> + (__force int)(opf & REQ_OP_MASK));
Please keep the enum req_op type instead of casting that type away with
"__force int".
Thanks,
Bart.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-05-01 14:42 ` Bart Van Assche
@ 2024-05-01 15:30 ` Ryusuke Konishi
2024-05-02 19:01 ` Ryusuke Konishi
0 siblings, 1 reply; 10+ messages in thread
From: Ryusuke Konishi @ 2024-05-01 15:30 UTC (permalink / raw
To: Bart Van Assche; +Cc: Andrew Morton, linux-nilfs, linux-kernel, Jens Axboe
On Wed, May 1, 2024 at 11:42 PM Bart Van Assche wrote:
>
> On 4/30/24 10:00, Ryusuke Konishi wrote:
> > trace_nilfs2_mdt_submit_block(inode, inode->i_ino, blkoff,
> > - opf & REQ_OP_MASK);
> > + (__force int)(opf & REQ_OP_MASK));
>
> Please keep the enum req_op type instead of casting that type away with
> "__force int".
>
> Thanks,
>
> Bart.
Hi Bart,
No, this type cast is necessary to prevent the following sparse warning:
CC [M] fs/nilfs2/mdt.o
CHECK fs/nilfs2/mdt.c
fs/nilfs2/mdt.c:155:43: warning: incorrect type in argument 4
(different base types)
fs/nilfs2/mdt.c:155:43: expected int mode
fs/nilfs2/mdt.c:155:43: got restricted blk_opf_t
What we're doing here is just changing the event tracing type back to
int, and keeping blk_opf_t and enum req_op in the rest of the code.
I understand if you have enough reason to ignore the warnings, but
Why do you have to keep enum req_op type instead of int for event tracing?
Regards,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-05-01 15:30 ` Ryusuke Konishi
@ 2024-05-02 19:01 ` Ryusuke Konishi
2024-05-05 12:47 ` Bart Van Assche
0 siblings, 1 reply; 10+ messages in thread
From: Ryusuke Konishi @ 2024-05-02 19:01 UTC (permalink / raw
To: Bart Van Assche; +Cc: Andrew Morton, linux-nilfs, linux-kernel, Jens Axboe
On Thu, May 2, 2024 at 12:30 AM Ryusuke Konishi wrote:
>
> On Wed, May 1, 2024 at 11:42 PM Bart Van Assche wrote:
> >
> > On 4/30/24 10:00, Ryusuke Konishi wrote:
> > > trace_nilfs2_mdt_submit_block(inode, inode->i_ino, blkoff,
> > > - opf & REQ_OP_MASK);
> > > + (__force int)(opf & REQ_OP_MASK));
> >
> > Please keep the enum req_op type instead of casting that type away with
> > "__force int".
> >
> > Thanks,
> >
> > Bart.
>
> Hi Bart,
>
> No, this type cast is necessary to prevent the following sparse warning:
>
> CC [M] fs/nilfs2/mdt.o
> CHECK fs/nilfs2/mdt.c
> fs/nilfs2/mdt.c:155:43: warning: incorrect type in argument 4
> (different base types)
> fs/nilfs2/mdt.c:155:43: expected int mode
> fs/nilfs2/mdt.c:155:43: got restricted blk_opf_t
>
> What we're doing here is just changing the event tracing type back to
> int, and keeping blk_opf_t and enum req_op in the rest of the code.
>
> I understand if you have enough reason to ignore the warnings, but
> Why do you have to keep enum req_op type instead of int for event tracing?
>
> Regards,
> Ryusuke Konishi
Hi Bart,
Sorry, I didn't realize you were digging into the issue and talking
with the sparse and kbuild teams to resolve the issue.
Is there any hope for a solution?
If you haven't given up yet on solving the underlying problem, I would
like to withdraw this patch.
Thanks,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-05-02 19:01 ` Ryusuke Konishi
@ 2024-05-05 12:47 ` Bart Van Assche
2024-05-05 19:04 ` Ryusuke Konishi
0 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2024-05-05 12:47 UTC (permalink / raw
To: Ryusuke Konishi; +Cc: Andrew Morton, linux-nilfs, linux-kernel, Jens Axboe
On 5/2/24 12:01 PM, Ryusuke Konishi wrote:
> If you haven't given up yet on solving the underlying problem, I would
> like to withdraw this patch.
Has this untested change been considered?
diff --git a/include/trace/events/nilfs2.h b/include/trace/events/nilfs2.h
index 8efc6236f57c..67fd2e002ca7 100644
--- a/include/trace/events/nilfs2.h
+++ b/include/trace/events/nilfs2.h
@@ -214,7 +214,7 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
__entry->inode,
__entry->ino,
__entry->blkoff,
- __entry->mode)
+ (__force u32)__entry->mode)
);
#endif /* _TRACE_NILFS2_H */
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-05-05 12:47 ` Bart Van Assche
@ 2024-05-05 19:04 ` Ryusuke Konishi
2024-05-06 17:26 ` Bart Van Assche
0 siblings, 1 reply; 10+ messages in thread
From: Ryusuke Konishi @ 2024-05-05 19:04 UTC (permalink / raw
To: Bart Van Assche; +Cc: Andrew Morton, linux-nilfs, linux-kernel, Jens Axboe
On Sun, May 5, 2024 at 9:47 PM Bart Van Assche wrote:
>
> On 5/2/24 12:01 PM, Ryusuke Konishi wrote:
> > If you haven't given up yet on solving the underlying problem, I would
> > like to withdraw this patch.
>
> Has this untested change been considered?
>
> diff --git a/include/trace/events/nilfs2.h b/include/trace/events/nilfs2.h
> index 8efc6236f57c..67fd2e002ca7 100644
> --- a/include/trace/events/nilfs2.h
> +++ b/include/trace/events/nilfs2.h
> @@ -214,7 +214,7 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
> __entry->inode,
> __entry->ino,
> __entry->blkoff,
> - __entry->mode)
> + (__force u32)__entry->mode)
> );
>
> #endif /* _TRACE_NILFS2_H */
No, I didn't think of that. There was no warning in TP_printk()
declaration of the nilfs2_mdt_submit_block trace point.
If you suggested this as an alternative idea, unfortunately the
following warnings are still output:
CC [M] fs/nilfs2/segment.o
CHECK fs/nilfs2/segment.c
fs/nilfs2/segment.c: note: in included file (through
include/trace/trace_events.h, include/trace/define_trace.h,
include/trace/events/nilfs2.h):
./include/trace/events/nilfs2.h:191:1: warning: cast to restricted blk_opf_t
./include/trace/events/nilfs2.h:191:1: warning: restricted blk_opf_t
degrades to integer
./include/trace/events/nilfs2.h:191:1: warning: restricted blk_opf_t
degrades to integer
I also tried typecasting on the declaration header side of event
tracing, but so far, the sparse warnings don't go away except for the
patch I first proposed.
But, better suggestions or solutions to the underlying problem are welcome.
(Again, should we put the patch on hold?)
Regards,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-05-05 19:04 ` Ryusuke Konishi
@ 2024-05-06 17:26 ` Bart Van Assche
2024-05-06 21:17 ` Ryusuke Konishi
0 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2024-05-06 17:26 UTC (permalink / raw
To: Ryusuke Konishi; +Cc: Andrew Morton, linux-nilfs, linux-kernel, Jens Axboe
On 5/5/24 12:04 PM, Ryusuke Konishi wrote:
> I also tried typecasting on the declaration header side of event
> tracing, but so far, the sparse warnings don't go away except for the
> patch I first proposed.
How about this patch?
diff --git a/include/trace/events/nilfs2.h b/include/trace/events/nilfs2.h
index 8efc6236f57c..b1a364a33a62 100644
--- a/include/trace/events/nilfs2.h
+++ b/include/trace/events/nilfs2.h
@@ -200,7 +200,11 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
__field(struct inode *, inode)
__field(unsigned long, ino)
__field(unsigned long, blkoff)
- __field(enum req_op, mode)
+ /*
+ * Use field_struct to avoid is_signed_type() on the bitwise
+ * type enum req_op.
+ */
+ __field_struct(enum req_op, mode)
),
TP_fast_assign(
Thanks,
Bart.
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header
2024-05-06 17:26 ` Bart Van Assche
@ 2024-05-06 21:17 ` Ryusuke Konishi
0 siblings, 0 replies; 10+ messages in thread
From: Ryusuke Konishi @ 2024-05-06 21:17 UTC (permalink / raw
To: Bart Van Assche; +Cc: Andrew Morton, linux-nilfs, linux-kernel, Jens Axboe
On Tue, May 7, 2024 at 2:26 AM Bart Van Assche wrote:
>
> On 5/5/24 12:04 PM, Ryusuke Konishi wrote:
> > I also tried typecasting on the declaration header side of event
> > tracing, but so far, the sparse warnings don't go away except for the
> > patch I first proposed.
>
> How about this patch?
>
> diff --git a/include/trace/events/nilfs2.h b/include/trace/events/nilfs2.h
> index 8efc6236f57c..b1a364a33a62 100644
> --- a/include/trace/events/nilfs2.h
> +++ b/include/trace/events/nilfs2.h
> @@ -200,7 +200,11 @@ TRACE_EVENT(nilfs2_mdt_submit_block,
> __field(struct inode *, inode)
> __field(unsigned long, ino)
> __field(unsigned long, blkoff)
> - __field(enum req_op, mode)
> + /*
> + * Use field_struct to avoid is_signed_type() on the bitwise
> + * type enum req_op.
> + */
> + __field_struct(enum req_op, mode)
> ),
>
> TP_fast_assign(
>
> Thanks,
>
> Bart.
Great! This patch removed the sparse warnings.
It passed multiple build methods and did not break either tracing or
nilfs2 functionality in operational testing.
I never thought of using __field_struct for enum type, but looking at
the definition it looks like it can be used to just avoid
is_signed_type() without any side effects. So, this patch looks
better than mine.
If you don't mind, could you complete this change as a patch by adding
a commit message and related tags?
Thanks,
Ryusuke Konishi
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-05-06 21:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 8:00 [PATCH -mm 0/2] nilfs2: reduce build warnings with "make C=1" Ryusuke Konishi
2024-04-30 8:00 ` [PATCH -mm 1/2] nilfs2: use integer type instead of enum req_op for event tracing header Ryusuke Konishi
2024-05-01 14:42 ` Bart Van Assche
2024-05-01 15:30 ` Ryusuke Konishi
2024-05-02 19:01 ` Ryusuke Konishi
2024-05-05 12:47 ` Bart Van Assche
2024-05-05 19:04 ` Ryusuke Konishi
2024-05-06 17:26 ` Bart Van Assche
2024-05-06 21:17 ` Ryusuke Konishi
2024-04-30 8:00 ` [PATCH -mm 2/2] nilfs2: make superblock data array index computation sparse friendly Ryusuke Konishi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).