* [PATCH 0/5] Block layer patches for kernel v5.8
@ 2020-05-16 0:19 Bart Van Assche
2020-05-16 0:19 ` [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument Bart Van Assche
` (4 more replies)
0 siblings, 5 replies; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 0:19 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Christoph Hellwig, Ming Lei, Bart Van Assche
Hi Jens,
The patches in this series are what I came up with as the result of
analyzing Alexander Potapenko's report about reading from null_blk.
Please consider these patches for kernel v5.8.
Thanks,
Bart.
Bart Van Assche (5):
block: Fix type of first compat_put_{,u}long() argument
bio.h: Declare the arguments of bio iteration functions const
block: Document the bio_vec properties
block: Fix zero_fill_bio()
null_blk: Zero-initialize read buffers in non-memory-backed mode
block/bio.c | 27 ++++++++++++++++++++++-----
block/ioctl.c | 4 ++--
drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
include/linux/bio.h | 7 ++++---
include/linux/bvec.h | 18 ++++++++++++++++--
5 files changed, 74 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument
2020-05-16 0:19 [PATCH 0/5] Block layer patches for kernel v5.8 Bart Van Assche
@ 2020-05-16 0:19 ` Bart Van Assche
2020-05-16 7:40 ` Arnd Bergmann
2020-05-16 12:41 ` Christoph Hellwig
2020-05-16 0:19 ` [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const Bart Van Assche
` (3 subsequent siblings)
4 siblings, 2 replies; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 0:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Bart Van Assche, Arnd Bergmann
This patch fixes the following sparse warnings:
block/ioctl.c:209:16: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:209:16: expected void const volatile [noderef] <asn:1> *
block/ioctl.c:209:16: got signed int [usertype] *argp
block/ioctl.c:214:16: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:214:16: expected void const volatile [noderef] <asn:1> *
block/ioctl.c:214:16: got unsigned int [usertype] *argp
block/ioctl.c:666:40: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:666:40: expected signed int [usertype] *argp
block/ioctl.c:666:40: got void [noderef] <asn:1> *argp
block/ioctl.c:672:41: warning: incorrect type in argument 1 (different address spaces)
block/ioctl.c:672:41: expected unsigned int [usertype] *argp
block/ioctl.c:672:41: got void [noderef] <asn:1> *argp
Cc: Arnd Bergmann <arnd@arndb.de>
Fixes: 9b81648cb5e3 ("compat_ioctl: simplify up block/ioctl.c")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
block/ioctl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/ioctl.c b/block/ioctl.c
index 75c64811b534..bdb3bbb253d9 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -204,12 +204,12 @@ static int put_u64(u64 __user *argp, u64 val)
}
#ifdef CONFIG_COMPAT
-static int compat_put_long(compat_long_t *argp, long val)
+static int compat_put_long(compat_long_t __user *argp, long val)
{
return put_user(val, argp);
}
-static int compat_put_ulong(compat_ulong_t *argp, compat_ulong_t val)
+static int compat_put_ulong(compat_ulong_t __user *argp, compat_ulong_t val)
{
return put_user(val, argp);
}
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const
2020-05-16 0:19 [PATCH 0/5] Block layer patches for kernel v5.8 Bart Van Assche
2020-05-16 0:19 ` [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument Bart Van Assche
@ 2020-05-16 0:19 ` Bart Van Assche
2020-05-16 8:55 ` Alexander Potapenko
` (2 more replies)
2020-05-16 0:19 ` [PATCH 3/5] block: Document the bio_vec properties Bart Van Assche
` (2 subsequent siblings)
4 siblings, 3 replies; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 0:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Bart Van Assche,
Damien Le Moal, Chaitanya Kulkarni, Alexander Potapenko
This change makes it possible to pass 'const struct bio *' arguments to
these functions.
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: Alexander Potapenko <glider@google.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
include/linux/bio.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/bio.h b/include/linux/bio.h
index a0ee494a6329..58e6134b1c05 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -70,7 +70,7 @@ static inline bool bio_has_data(struct bio *bio)
return false;
}
-static inline bool bio_no_advance_iter(struct bio *bio)
+static inline bool bio_no_advance_iter(const struct bio *bio)
{
return bio_op(bio) == REQ_OP_DISCARD ||
bio_op(bio) == REQ_OP_SECURE_ERASE ||
@@ -138,8 +138,8 @@ static inline bool bio_next_segment(const struct bio *bio,
#define bio_for_each_segment_all(bvl, bio, iter) \
for (bvl = bvec_init_iter_all(&iter); bio_next_segment((bio), &iter); )
-static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
- unsigned bytes)
+static inline void bio_advance_iter(const struct bio *bio,
+ struct bvec_iter *iter, unsigned bytes)
{
iter->bi_sector += bytes >> 9;
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/5] block: Document the bio_vec properties
2020-05-16 0:19 [PATCH 0/5] Block layer patches for kernel v5.8 Bart Van Assche
2020-05-16 0:19 ` [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument Bart Van Assche
2020-05-16 0:19 ` [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const Bart Van Assche
@ 2020-05-16 0:19 ` Bart Van Assche
2020-05-16 12:43 ` Christoph Hellwig
2020-05-16 0:19 ` [PATCH 4/5] block: Fix zero_fill_bio() Bart Van Assche
2020-05-16 0:19 ` [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode Bart Van Assche
4 siblings, 1 reply; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 0:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Bart Van Assche,
Christoph Hellwig
Since it is nontrivial that nth_page() does not have to be used for a
bio_vec, document this.
CC: Christoph Hellwig <hch@infradead.org>
Cc: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
include/linux/bvec.h | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index a81c13ac1972..25295c11b164 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -12,8 +12,22 @@
#include <linux/errno.h>
#include <linux/mm.h>
-/*
- * was unsigned short, but we might as well be ready for > 64kB I/O pages
+/**
+ * struct bio_vec - a contiguous range of physical memory addresses
+ * @bv_page: First page associated with the address range.
+ * @bv_len: Number of bytes in the address range.
+ * @bv_offset: Start of the address range relative to the start of @bv_page.
+ *
+ * The following holds for a bvec if n * PAGE_SIZE < bv_offset + bv_len:
+ *
+ * nth_page(@bv_page, n) == @bv_page + n
+ *
+ * From the description of commit 52d52d1c98a9 ("block: only allow contiguous
+ * page structs in a bio_vec"): "We currently have to call nth_page when
+ * iterating over pages inside a bio_vec. Jens complained a while ago that
+ * this is fairly expensive. To mitigate this we can check that that the
+ * actual page structures are contiguous when adding them to the bio, and just
+ * do check pointer arithmetics later on." See also page_is_mergeable().
*/
struct bio_vec {
struct page *bv_page;
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/5] block: Fix zero_fill_bio()
2020-05-16 0:19 [PATCH 0/5] Block layer patches for kernel v5.8 Bart Van Assche
` (2 preceding siblings ...)
2020-05-16 0:19 ` [PATCH 3/5] block: Document the bio_vec properties Bart Van Assche
@ 2020-05-16 0:19 ` Bart Van Assche
2020-05-16 5:50 ` Ming Lei
2020-05-16 0:19 ` [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode Bart Van Assche
4 siblings, 1 reply; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 0:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Bart Van Assche, stable
Multiple block drivers use zero_fill_bio() to zero-initialize the data
buffer used for read operations. Make sure that all pages are zeroed
instead of only the first if one or more multi-page bvecs are used to
describe the data buffer.
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
block/bio.c | 27 ++++++++++++++++++++++-----
include/linux/bio.h | 1 +
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 1594804fe8bc..48fcafbdae70 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -527,17 +527,34 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
}
EXPORT_SYMBOL(bio_alloc_bioset);
+void zero_fill_bvec(const struct bio_vec *bvec)
+{
+ struct page *page = bvec->bv_page;
+ u32 offset = bvec->bv_offset;
+ u32 left = bvec->bv_len;
+
+ while (left) {
+ u32 len = min_t(u32, left, PAGE_SIZE - offset);
+ void *kaddr;
+
+ kaddr = kmap_atomic(page);
+ memset(kaddr + offset, 0, len);
+ flush_dcache_page(page);
+ kunmap_atomic(kaddr);
+ page++;
+ left -= len;
+ offset = 0;
+ }
+}
+EXPORT_SYMBOL(zero_fill_bvec);
+
void zero_fill_bio_iter(struct bio *bio, struct bvec_iter start)
{
- unsigned long flags;
struct bio_vec bv;
struct bvec_iter iter;
__bio_for_each_segment(bv, bio, iter, start) {
- char *data = bvec_kmap_irq(&bv, &flags);
- memset(data, 0, bv.bv_len);
- flush_dcache_page(bv.bv_page);
- bvec_kunmap_irq(data, &flags);
+ zero_fill_bvec(&bv);
}
}
EXPORT_SYMBOL(zero_fill_bio_iter);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 58e6134b1c05..9438cbdfa19e 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -455,6 +455,7 @@ extern void bio_copy_data_iter(struct bio *dst, struct bvec_iter *dst_iter,
extern void bio_copy_data(struct bio *dst, struct bio *src);
extern void bio_list_copy_data(struct bio *dst, struct bio *src);
extern void bio_free_pages(struct bio *bio);
+void zero_fill_bvec(const struct bio_vec *bvec);
void zero_fill_bio_iter(struct bio *bio, struct bvec_iter iter);
void bio_truncate(struct bio *bio, unsigned new_size);
void guard_bio_eod(struct bio *bio);
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-16 0:19 [PATCH 0/5] Block layer patches for kernel v5.8 Bart Van Assche
` (3 preceding siblings ...)
2020-05-16 0:19 ` [PATCH 4/5] block: Fix zero_fill_bio() Bart Van Assche
@ 2020-05-16 0:19 ` Bart Van Assche
2020-05-16 9:40 ` Alexander Potapenko
2020-05-18 1:12 ` Damien Le Moal
4 siblings, 2 replies; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 0:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Bart Van Assche,
Damien Le Moal, Chaitanya Kulkarni, Alexander Potapenko
This patch suppresses an uninteresting KMSAN complaint without affecting
performance of the null_blk driver if CONFIG_KMSAN is disabled.
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Cc: Alexander Potapenko <glider@google.com>
Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
index 06f5761fccb6..df1e144eeaa4 100644
--- a/drivers/block/null_blk_main.c
+++ b/drivers/block/null_blk_main.c
@@ -1250,8 +1250,38 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
return errno_to_blk_status(err);
}
+static void nullb_zero_rq_data_buffer(const struct request *rq)
+{
+ struct req_iterator iter;
+ struct bio_vec bvec;
+
+ rq_for_each_bvec(bvec, rq, iter)
+ zero_fill_bvec(&bvec);
+}
+
+static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
+{
+ struct nullb_device *dev = cmd->nq->dev;
+
+ if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
+ zero_fill_bio(cmd->bio);
+ else if (req_op(cmd->rq) == REQ_OP_READ)
+ nullb_zero_rq_data_buffer(cmd->rq);
+}
+
+/* Complete a request. Only called if dev->memory_backed == 0. */
static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
{
+ /*
+ * Since root privileges are required to configure the null_blk
+ * driver, it is fine that this driver does not initialize the
+ * data buffers of read commands. Zero-initialize these buffers
+ * anyway if KMSAN is enabled to prevent that KMSAN complains
+ * about null_blk not initializing read data buffers.
+ */
+ if (IS_ENABLED(CONFIG_KMSAN))
+ nullb_zero_read_cmd_buffer(cmd);
+
/* Complete IO by inline, softirq or timer */
switch (cmd->nq->dev->irqmode) {
case NULL_IRQ_SOFTIRQ:
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 4/5] block: Fix zero_fill_bio()
2020-05-16 0:19 ` [PATCH 4/5] block: Fix zero_fill_bio() Bart Van Assche
@ 2020-05-16 5:50 ` Ming Lei
0 siblings, 0 replies; 23+ messages in thread
From: Ming Lei @ 2020-05-16 5:50 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, stable
On Fri, May 15, 2020 at 05:19:13PM -0700, Bart Van Assche wrote:
> Multiple block drivers use zero_fill_bio() to zero-initialize the data
> buffer used for read operations. Make sure that all pages are zeroed
> instead of only the first if one or more multi-page bvecs are used to
> describe the data buffer.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> block/bio.c | 27 ++++++++++++++++++++++-----
> include/linux/bio.h | 1 +
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 1594804fe8bc..48fcafbdae70 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -527,17 +527,34 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, unsigned int nr_iovecs,
> }
> EXPORT_SYMBOL(bio_alloc_bioset);
>
> +void zero_fill_bvec(const struct bio_vec *bvec)
> +{
> + struct page *page = bvec->bv_page;
> + u32 offset = bvec->bv_offset;
> + u32 left = bvec->bv_len;
> +
> + while (left) {
> + u32 len = min_t(u32, left, PAGE_SIZE - offset);
> + void *kaddr;
> +
> + kaddr = kmap_atomic(page);
> + memset(kaddr + offset, 0, len);
> + flush_dcache_page(page);
> + kunmap_atomic(kaddr);
> + page++;
> + left -= len;
> + offset = 0;
> + }
> +}
> +EXPORT_SYMBOL(zero_fill_bvec);
> +
> void zero_fill_bio_iter(struct bio *bio, struct bvec_iter start)
> {
> - unsigned long flags;
> struct bio_vec bv;
> struct bvec_iter iter;
>
> __bio_for_each_segment(bv, bio, iter, start) {
The 'bv' from __bio_for_each_segment() is single page bvec, and so far
only [__]bio_for_each_bvec iterates over multi-page bvec.
So nothing to be fixed and this patch isn't necessary.
Thanks,
Ming
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument
2020-05-16 0:19 ` [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument Bart Van Assche
@ 2020-05-16 7:40 ` Arnd Bergmann
2020-05-16 12:41 ` Christoph Hellwig
1 sibling, 0 replies; 23+ messages in thread
From: Arnd Bergmann @ 2020-05-16 7:40 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei
On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This patch fixes the following sparse warnings:
>
> block/ioctl.c:209:16: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:209:16: expected void const volatile [noderef] <asn:1> *
> block/ioctl.c:209:16: got signed int [usertype] *argp
> block/ioctl.c:214:16: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:214:16: expected void const volatile [noderef] <asn:1> *
> block/ioctl.c:214:16: got unsigned int [usertype] *argp
> block/ioctl.c:666:40: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:666:40: expected signed int [usertype] *argp
> block/ioctl.c:666:40: got void [noderef] <asn:1> *argp
> block/ioctl.c:672:41: warning: incorrect type in argument 1 (different address spaces)
> block/ioctl.c:672:41: expected unsigned int [usertype] *argp
> block/ioctl.c:672:41: got void [noderef] <asn:1> *argp
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Fixes: 9b81648cb5e3 ("compat_ioctl: simplify up block/ioctl.c")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Thanks for the fix!
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const
2020-05-16 0:19 ` [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const Bart Van Assche
@ 2020-05-16 8:55 ` Alexander Potapenko
2020-05-16 17:10 ` Bart Van Assche
2020-05-16 12:42 ` Christoph Hellwig
2020-05-17 22:46 ` Chaitanya Kulkarni
2 siblings, 1 reply; 23+ messages in thread
From: Alexander Potapenko @ 2020-05-16 8:55 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei,
Damien Le Moal, Chaitanya Kulkarni
On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This change makes it possible to pass 'const struct bio *' arguments to
> these functions.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko <glider@google.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> include/linux/bio.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index a0ee494a6329..58e6134b1c05 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -70,7 +70,7 @@ static inline bool bio_has_data(struct bio *bio)
> return false;
> }
>
> -static inline bool bio_no_advance_iter(struct bio *bio)
> +static inline bool bio_no_advance_iter(const struct bio *bio)
> {
> return bio_op(bio) == REQ_OP_DISCARD ||
> bio_op(bio) == REQ_OP_SECURE_ERASE ||
> @@ -138,8 +138,8 @@ static inline bool bio_next_segment(const struct bio *bio,
> #define bio_for_each_segment_all(bvl, bio, iter) \
> for (bvl = bvec_init_iter_all(&iter); bio_next_segment((bio), &iter); )
>
> -static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
> - unsigned bytes)
> +static inline void bio_advance_iter(const struct bio *bio,
> + struct bvec_iter *iter, unsigned bytes)
> {
> iter->bi_sector += bytes >> 9;
On a related note, should this 9 be SECTOR_SHIFT?
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-16 0:19 ` [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode Bart Van Assche
@ 2020-05-16 9:40 ` Alexander Potapenko
2020-05-18 1:12 ` Damien Le Moal
1 sibling, 0 replies; 23+ messages in thread
From: Alexander Potapenko @ 2020-05-16 9:40 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei,
Damien Le Moal, Chaitanya Kulkarni
On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This patch suppresses an uninteresting KMSAN complaint without affecting
> performance of the null_blk driver if CONFIG_KMSAN is disabled.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko <glider@google.com>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Tested-by: Alexander Potapenko <glider@google.com>
> ---
> drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 06f5761fccb6..df1e144eeaa4 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1250,8 +1250,38 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
> return errno_to_blk_status(err);
> }
>
> +static void nullb_zero_rq_data_buffer(const struct request *rq)
> +{
> + struct req_iterator iter;
> + struct bio_vec bvec;
> +
> + rq_for_each_bvec(bvec, rq, iter)
> + zero_fill_bvec(&bvec);
> +}
> +
> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
> +{
> + struct nullb_device *dev = cmd->nq->dev;
> +
> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
> + zero_fill_bio(cmd->bio);
> + else if (req_op(cmd->rq) == REQ_OP_READ)
> + nullb_zero_rq_data_buffer(cmd->rq);
> +}
> +
> +/* Complete a request. Only called if dev->memory_backed == 0. */
> static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
> {
> + /*
> + * Since root privileges are required to configure the null_blk
> + * driver, it is fine that this driver does not initialize the
> + * data buffers of read commands. Zero-initialize these buffers
> + * anyway if KMSAN is enabled to prevent that KMSAN complains
> + * about null_blk not initializing read data buffers.
> + */
> + if (IS_ENABLED(CONFIG_KMSAN))
> + nullb_zero_read_cmd_buffer(cmd);
> +
> /* Complete IO by inline, softirq or timer */
> switch (cmd->nq->dev->irqmode) {
> case NULL_IRQ_SOFTIRQ:
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument
2020-05-16 0:19 ` [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument Bart Van Assche
2020-05-16 7:40 ` Arnd Bergmann
@ 2020-05-16 12:41 ` Christoph Hellwig
1 sibling, 0 replies; 23+ messages in thread
From: Christoph Hellwig @ 2020-05-16 12:41 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei, Arnd Bergmann
On Fri, May 15, 2020 at 05:19:10PM -0700, Bart Van Assche wrote:
> This patch fixes the following sparse warnings:
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const
2020-05-16 0:19 ` [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const Bart Van Assche
2020-05-16 8:55 ` Alexander Potapenko
@ 2020-05-16 12:42 ` Christoph Hellwig
2020-05-17 22:46 ` Chaitanya Kulkarni
2 siblings, 0 replies; 23+ messages in thread
From: Christoph Hellwig @ 2020-05-16 12:42 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei,
Damien Le Moal, Chaitanya Kulkarni, Alexander Potapenko
On Fri, May 15, 2020 at 05:19:11PM -0700, Bart Van Assche wrote:
> This change makes it possible to pass 'const struct bio *' arguments to
> these functions.
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/5] block: Document the bio_vec properties
2020-05-16 0:19 ` [PATCH 3/5] block: Document the bio_vec properties Bart Van Assche
@ 2020-05-16 12:43 ` Christoph Hellwig
0 siblings, 0 replies; 23+ messages in thread
From: Christoph Hellwig @ 2020-05-16 12:43 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei, Christoph Hellwig
On Fri, May 15, 2020 at 05:19:12PM -0700, Bart Van Assche wrote:
> Since it is nontrivial that nth_page() does not have to be used for a
> bio_vec, document this.
No need to quote the commit in the source code, just add the expanation
and not the source in the commit log for this patch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const
2020-05-16 8:55 ` Alexander Potapenko
@ 2020-05-16 17:10 ` Bart Van Assche
0 siblings, 0 replies; 23+ messages in thread
From: Bart Van Assche @ 2020-05-16 17:10 UTC (permalink / raw)
To: Alexander Potapenko
Cc: Jens Axboe, linux-block, Christoph Hellwig, Ming Lei,
Damien Le Moal, Chaitanya Kulkarni
On 2020-05-16 01:55, Alexander Potapenko wrote:
> On Sat, May 16, 2020 at 2:19 AM Bart Van Assche <bvanassche@acm.org> wrote:
>> -static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>> - unsigned bytes)
>> +static inline void bio_advance_iter(const struct bio *bio,
>> + struct bvec_iter *iter, unsigned bytes)
>> {
>> iter->bi_sector += bytes >> 9;
>
> On a related note, should this 9 be SECTOR_SHIFT?
Hi Alexander,
I think a patch series is already under review for replacing "9" with
SECTOR_SHIFT. See also
https://lore.kernel.org/linux-block/20200507075100.1779-1-thunder.leizhen@huawei.com/.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const
2020-05-16 0:19 ` [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const Bart Van Assche
2020-05-16 8:55 ` Alexander Potapenko
2020-05-16 12:42 ` Christoph Hellwig
@ 2020-05-17 22:46 ` Chaitanya Kulkarni
2 siblings, 0 replies; 23+ messages in thread
From: Chaitanya Kulkarni @ 2020-05-17 22:46 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Damien Le Moal,
Alexander Potapenko
On 5/15/20 5:19 PM, Bart Van Assche wrote:
> This change makes it possible to pass 'const struct bio *' arguments to
> these functions.
>
> Cc: Christoph Hellwig<hch@lst.de>
> Cc: Ming Lei<ming.lei@redhat.com>
> Cc: Damien Le Moal<damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni<chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko<glider@google.com>
> Signed-off-by: Bart Van Assche<bvanassche@acm.org>
Looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-16 0:19 ` [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode Bart Van Assche
2020-05-16 9:40 ` Alexander Potapenko
@ 2020-05-18 1:12 ` Damien Le Moal
2020-05-18 1:32 ` Bart Van Assche
1 sibling, 1 reply; 23+ messages in thread
From: Damien Le Moal @ 2020-05-18 1:12 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020/05/16 9:19, Bart Van Assche wrote:
> This patch suppresses an uninteresting KMSAN complaint without affecting
> performance of the null_blk driver if CONFIG_KMSAN is disabled.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Damien Le Moal <damien.lemoal@wdc.com>
> Cc: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> Cc: Alexander Potapenko <glider@google.com>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/block/null_blk_main.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c
> index 06f5761fccb6..df1e144eeaa4 100644
> --- a/drivers/block/null_blk_main.c
> +++ b/drivers/block/null_blk_main.c
> @@ -1250,8 +1250,38 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
> return errno_to_blk_status(err);
> }
>
> +static void nullb_zero_rq_data_buffer(const struct request *rq)
> +{
> + struct req_iterator iter;
> + struct bio_vec bvec;
> +
> + rq_for_each_bvec(bvec, rq, iter)
> + zero_fill_bvec(&bvec);
> +}
> +
> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
> +{
> + struct nullb_device *dev = cmd->nq->dev;
> +
> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
> + zero_fill_bio(cmd->bio);
> + else if (req_op(cmd->rq) == REQ_OP_READ)
> + nullb_zero_rq_data_buffer(cmd->rq);
> +}
Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
> +
> +/* Complete a request. Only called if dev->memory_backed == 0. */
> static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
> {
> + /*
> + * Since root privileges are required to configure the null_blk
> + * driver, it is fine that this driver does not initialize the
> + * data buffers of read commands. Zero-initialize these buffers
> + * anyway if KMSAN is enabled to prevent that KMSAN complains
> + * about null_blk not initializing read data buffers.
> + */
> + if (IS_ENABLED(CONFIG_KMSAN))
> + nullb_zero_read_cmd_buffer(cmd);
> +
> /* Complete IO by inline, softirq or timer */
> switch (cmd->nq->dev->irqmode) {
> case NULL_IRQ_SOFTIRQ:
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-18 1:12 ` Damien Le Moal
@ 2020-05-18 1:32 ` Bart Van Assche
2020-05-18 2:10 ` Damien Le Moal
0 siblings, 1 reply; 23+ messages in thread
From: Bart Van Assche @ 2020-05-18 1:32 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020-05-17 18:12, Damien Le Moal wrote:
> On 2020/05/16 9:19, Bart Van Assche wrote:
>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>> +{
>> + struct req_iterator iter;
>> + struct bio_vec bvec;
>> +
>> + rq_for_each_bvec(bvec, rq, iter)
>> + zero_fill_bvec(&bvec);
>> +}
>> +
>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>> +{
>> + struct nullb_device *dev = cmd->nq->dev;
>> +
>> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>> + zero_fill_bio(cmd->bio);
>> + else if (req_op(cmd->rq) == REQ_OP_READ)
>> + nullb_zero_rq_data_buffer(cmd->rq);
>> +}
>
> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
Hi Damien,
It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
#ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
expose the above code to the build robot.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-18 1:32 ` Bart Van Assche
@ 2020-05-18 2:10 ` Damien Le Moal
2020-05-18 2:56 ` Bart Van Assche
0 siblings, 1 reply; 23+ messages in thread
From: Damien Le Moal @ 2020-05-18 2:10 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020/05/18 10:32, Bart Van Assche wrote:
> On 2020-05-17 18:12, Damien Le Moal wrote:
>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>> +{
>>> + struct req_iterator iter;
>>> + struct bio_vec bvec;
>>> +
>>> + rq_for_each_bvec(bvec, rq, iter)
>>> + zero_fill_bvec(&bvec);
>>> +}
>>> +
>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>> +{
>>> + struct nullb_device *dev = cmd->nq->dev;
>>> +
>>> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>> + zero_fill_bio(cmd->bio);
>>> + else if (req_op(cmd->rq) == REQ_OP_READ)
>>> + nullb_zero_rq_data_buffer(cmd->rq);
>>> +}
>>
>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>
> Hi Damien,
>
> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
> expose the above code to the build robot.
But then you will get a "defined but unused" build warning, no ?
>
> Thanks,
>
> Bart.
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-18 2:10 ` Damien Le Moal
@ 2020-05-18 2:56 ` Bart Van Assche
2020-05-18 3:12 ` Damien Le Moal
0 siblings, 1 reply; 23+ messages in thread
From: Bart Van Assche @ 2020-05-18 2:56 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020-05-17 19:10, Damien Le Moal wrote:
> On 2020/05/18 10:32, Bart Van Assche wrote:
>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>> +{
>>>> + struct req_iterator iter;
>>>> + struct bio_vec bvec;
>>>> +
>>>> + rq_for_each_bvec(bvec, rq, iter)
>>>> + zero_fill_bvec(&bvec);
>>>> +}
>>>> +
>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>> +{
>>>> + struct nullb_device *dev = cmd->nq->dev;
>>>> +
>>>> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>> + zero_fill_bio(cmd->bio);
>>>> + else if (req_op(cmd->rq) == REQ_OP_READ)
>>>> + nullb_zero_rq_data_buffer(cmd->rq);
>>>> +}
>>>
>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>
>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>> expose the above code to the build robot.
>
> But then you will get a "defined but unused" build warning, no ?
Not when using IS_ENABLED(...).
Bart.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-18 2:56 ` Bart Van Assche
@ 2020-05-18 3:12 ` Damien Le Moal
2020-05-18 14:31 ` Bart Van Assche
0 siblings, 1 reply; 23+ messages in thread
From: Damien Le Moal @ 2020-05-18 3:12 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020/05/18 11:56, Bart Van Assche wrote:
> On 2020-05-17 19:10, Damien Le Moal wrote:
>> On 2020/05/18 10:32, Bart Van Assche wrote:
>>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>>> +{
>>>>> + struct req_iterator iter;
>>>>> + struct bio_vec bvec;
>>>>> +
>>>>> + rq_for_each_bvec(bvec, rq, iter)
>>>>> + zero_fill_bvec(&bvec);
>>>>> +}
>>>>> +
>>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>>> +{
>>>>> + struct nullb_device *dev = cmd->nq->dev;
>>>>> +
>>>>> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>>> + zero_fill_bio(cmd->bio);
>>>>> + else if (req_op(cmd->rq) == REQ_OP_READ)
>>>>> + nullb_zero_rq_data_buffer(cmd->rq);
>>>>> +}
>>>>
>>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>>
>>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>>> expose the above code to the build robot.
>>
>> But then you will get a "defined but unused" build warning, no ?
>
> Not when using IS_ENABLED(...).
I do not understand: the "if (IS_ENABLED(CONFIG_KMSAN))" will be compiled out if
CONFIG_KMSAN is not enabled/defined, but the function definitions will still
remain, won't they ? That will lead to "defined but unused" warning, no ? What
am I missing here ?
>
> Bart.
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-18 3:12 ` Damien Le Moal
@ 2020-05-18 14:31 ` Bart Van Assche
2020-05-19 3:03 ` Damien Le Moal
0 siblings, 1 reply; 23+ messages in thread
From: Bart Van Assche @ 2020-05-18 14:31 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020-05-17 20:12, Damien Le Moal wrote:
> On 2020/05/18 11:56, Bart Van Assche wrote:
>> On 2020-05-17 19:10, Damien Le Moal wrote:
>>> On 2020/05/18 10:32, Bart Van Assche wrote:
>>>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>>>> +{
>>>>>> + struct req_iterator iter;
>>>>>> + struct bio_vec bvec;
>>>>>> +
>>>>>> + rq_for_each_bvec(bvec, rq, iter)
>>>>>> + zero_fill_bvec(&bvec);
>>>>>> +}
>>>>>> +
>>>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>>>> +{
>>>>>> + struct nullb_device *dev = cmd->nq->dev;
>>>>>> +
>>>>>> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>>>> + zero_fill_bio(cmd->bio);
>>>>>> + else if (req_op(cmd->rq) == REQ_OP_READ)
>>>>>> + nullb_zero_rq_data_buffer(cmd->rq);
>>>>>> +}
>>>>>
>>>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>>>
>>>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>>>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>>>> expose the above code to the build robot.
>>>
>>> But then you will get a "defined but unused" build warning, no ?
>>
>> Not when using IS_ENABLED(...).
>
> I do not understand: the "if (IS_ENABLED(CONFIG_KMSAN))" will be compiled out if
> CONFIG_KMSAN is not enabled/defined, but the function definitions will still
> remain, won't they ? That will lead to "defined but unused" warning, no ? What
> am I missing here ?
"if (IS_ENABLED(CONFIG_KMSAN))" won't be removed by the preprocessor.
The preprocessor will convert it into if (0).
This is what I found in the gcc documentation about -Wunused-function:
"-Wunused-function
Warn whenever a static function is declared but not defined or a
non-inline static function is unused. This warning is enabled by -Wall."
I think that "if (0) func(...)" counts as using func().
Bart.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-18 14:31 ` Bart Van Assche
@ 2020-05-19 3:03 ` Damien Le Moal
2020-05-19 4:10 ` Bart Van Assche
0 siblings, 1 reply; 23+ messages in thread
From: Damien Le Moal @ 2020-05-19 3:03 UTC (permalink / raw)
To: Bart Van Assche, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020/05/18 23:31, Bart Van Assche wrote:
> On 2020-05-17 20:12, Damien Le Moal wrote:
>> On 2020/05/18 11:56, Bart Van Assche wrote:
>>> On 2020-05-17 19:10, Damien Le Moal wrote:
>>>> On 2020/05/18 10:32, Bart Van Assche wrote:
>>>>> On 2020-05-17 18:12, Damien Le Moal wrote:
>>>>>> On 2020/05/16 9:19, Bart Van Assche wrote:
>>>>>>> +static void nullb_zero_rq_data_buffer(const struct request *rq)
>>>>>>> +{
>>>>>>> + struct req_iterator iter;
>>>>>>> + struct bio_vec bvec;
>>>>>>> +
>>>>>>> + rq_for_each_bvec(bvec, rq, iter)
>>>>>>> + zero_fill_bvec(&bvec);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void nullb_zero_read_cmd_buffer(struct nullb_cmd *cmd)
>>>>>>> +{
>>>>>>> + struct nullb_device *dev = cmd->nq->dev;
>>>>>>> +
>>>>>>> + if (dev->queue_mode == NULL_Q_BIO && bio_op(cmd->bio) == REQ_OP_READ)
>>>>>>> + zero_fill_bio(cmd->bio);
>>>>>>> + else if (req_op(cmd->rq) == REQ_OP_READ)
>>>>>>> + nullb_zero_rq_data_buffer(cmd->rq);
>>>>>>> +}
>>>>>>
>>>>>> Shouldn't the definition of these two functions be under a "#ifdef CONFIG_KMSAN" ?
>>>>>
>>>>> It is on purpose that I used IS_ENABLED(CONFIG_KMSAN) below instead of
>>>>> #ifdef CONFIG_KMSAN. CONFIG_KMSAN is not yet upstream and I want to
>>>>> expose the above code to the build robot.
>>>>
>>>> But then you will get a "defined but unused" build warning, no ?
>>>
>>> Not when using IS_ENABLED(...).
>>
>> I do not understand: the "if (IS_ENABLED(CONFIG_KMSAN))" will be compiled out if
>> CONFIG_KMSAN is not enabled/defined, but the function definitions will still
>> remain, won't they ? That will lead to "defined but unused" warning, no ? What
>> am I missing here ?
>
> "if (IS_ENABLED(CONFIG_KMSAN))" won't be removed by the preprocessor.
> The preprocessor will convert it into if (0).
>
> This is what I found in the gcc documentation about -Wunused-function:
> "-Wunused-function
> Warn whenever a static function is declared but not defined or a
> non-inline static function is unused. This warning is enabled by -Wall."
> I think that "if (0) func(...)" counts as using func().
Makes sense. Thanks for the explanation.
But from code-size perspective, I think it would still make sense to add the
#ifdef CONFIG_KMSAN around the zeroing functions.
>
> Bart.
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode
2020-05-19 3:03 ` Damien Le Moal
@ 2020-05-19 4:10 ` Bart Van Assche
0 siblings, 0 replies; 23+ messages in thread
From: Bart Van Assche @ 2020-05-19 4:10 UTC (permalink / raw)
To: Damien Le Moal, Jens Axboe
Cc: linux-block, Christoph Hellwig, Ming Lei, Chaitanya Kulkarni,
Alexander Potapenko
On 2020-05-18 20:03, Damien Le Moal wrote:
> Makes sense. Thanks for the explanation.
> But from code-size perspective, I think it would still make sense to add the
> #ifdef CONFIG_KMSAN around the zeroing functions.
Does anyone who cares about kernel size include the null_blk driver? I
would be surprised if anyone who cares about kernel size (e.g. embedded
systems developers) would use anything else than CONFIG_BLK_DEV_NULL_BLK=n.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-05-19 4:10 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-16 0:19 [PATCH 0/5] Block layer patches for kernel v5.8 Bart Van Assche
2020-05-16 0:19 ` [PATCH 1/5] block: Fix type of first compat_put_{,u}long() argument Bart Van Assche
2020-05-16 7:40 ` Arnd Bergmann
2020-05-16 12:41 ` Christoph Hellwig
2020-05-16 0:19 ` [PATCH 2/5] bio.h: Declare the arguments of bio iteration functions const Bart Van Assche
2020-05-16 8:55 ` Alexander Potapenko
2020-05-16 17:10 ` Bart Van Assche
2020-05-16 12:42 ` Christoph Hellwig
2020-05-17 22:46 ` Chaitanya Kulkarni
2020-05-16 0:19 ` [PATCH 3/5] block: Document the bio_vec properties Bart Van Assche
2020-05-16 12:43 ` Christoph Hellwig
2020-05-16 0:19 ` [PATCH 4/5] block: Fix zero_fill_bio() Bart Van Assche
2020-05-16 5:50 ` Ming Lei
2020-05-16 0:19 ` [PATCH 5/5] null_blk: Zero-initialize read buffers in non-memory-backed mode Bart Van Assche
2020-05-16 9:40 ` Alexander Potapenko
2020-05-18 1:12 ` Damien Le Moal
2020-05-18 1:32 ` Bart Van Assche
2020-05-18 2:10 ` Damien Le Moal
2020-05-18 2:56 ` Bart Van Assche
2020-05-18 3:12 ` Damien Le Moal
2020-05-18 14:31 ` Bart Van Assche
2020-05-19 3:03 ` Damien Le Moal
2020-05-19 4:10 ` Bart Van Assche
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 NNTP newsgroup(s).