diff mbox series

btrfs: fix CHECK_INTEGRITY warning when !QUEUE_FLAG_WC

Message ID 20211022082637.50880-1-wangyugui@e16-tech.com (mailing list archive)
State New, archived
Headers show
Series btrfs: fix CHECK_INTEGRITY warning when !QUEUE_FLAG_WC | expand

Commit Message

Wang Yugui Oct. 22, 2021, 8:26 a.m. UTC 
xfstest/btrfs/220 tigger a check-integrity warning when
1) CONFIG_BTRFS_FS_CHECK_INTEGRITY=y
2) xfstest/btrfs/220 run on a disk with WCE=1

In write_dev_flush(), submit_bio(REQ_SYNC | REQ_PREFLUSH) can be skipped when
!QUEUE_FLAG_WC. but btrfsic_submit_bio() != submit_bio() when CONFIG_BTRFS_FS_CHECK_INTEGRITY

Signed-off-by: wangyugui <wangyugui@e16-tech.com>
---
 fs/btrfs/disk-io.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Filipe Manana Oct. 22, 2021, 2:42 p.m. UTC | #1 
On Fri, Oct 22, 2021 at 9:29 AM wangyugui <wangyugui@e16-tech.com> wrote:
>
> xfstest/btrfs/220 tigger a check-integrity warning when
> 1) CONFIG_BTRFS_FS_CHECK_INTEGRITY=y
> 2) xfstest/btrfs/220 run on a disk with WCE=1
>
> In write_dev_flush(), submit_bio(REQ_SYNC | REQ_PREFLUSH) can be skipped when
> !QUEUE_FLAG_WC. but btrfsic_submit_bio() != submit_bio() when CONFIG_BTRFS_FS_CHECK_INTEGRITY

The change log is a bit confusing, something more detailed like this
would be better:

When a disk has write caching disabled, we skip submission of a bio
with flush and sync requests before
writing the superblock, since it's not needed. However when the
integrity checker is enabled, this results in
reports that there are metadata blocks referred by a superblock that
were not properly flushed. So don't
skip the bio submission only when the integrity checker is enabled for
the sake of simplicity, since this is
a debug tool and not meant for use in non-debug builds.

Btw, the line length of a changelog should not exceed 75 characters.

For reference, you could also paste the warning you got in the change
log, it makes it easier
to grep for fixes for example.

>
> Signed-off-by: wangyugui <wangyugui@e16-tech.com>
> ---
>  fs/btrfs/disk-io.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 355ea88..7b17357 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3971,8 +3971,14 @@ static void write_dev_flush(struct btrfs_device *device)
>         struct request_queue *q = bdev_get_queue(device->bdev);
>         struct bio *bio = device->flush_bio;
>
> +       #ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
> +       /*
> +       * submit_bio(REQ_SYNC | REQ_PREFLUSH) can be skipped when !QUEUE_FLAG_WC.
> +       * but btrfsic_submit_bio() != submit_bio() when CONFIG_BTRFS_FS_CHECK_INTEGRITY
> +       */

The comment could use the same type of improvement as well.

>         if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
>                 return;
> +       #endif

As for the code itself, it looks good.

Thanks.

>
>         bio_reset(bio);
>         bio->bi_end_io = btrfs_end_empty_barrier;
> --
> 2.32.0
>
kernel test robot Nov. 2, 2021, 5:32 a.m. UTC | #2 
Hi wangyugui,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.15 next-20211101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/wangyugui/btrfs-fix-CHECK_INTEGRITY-warning-when-QUEUE_FLAG_WC/20211022-162718
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/ced40ee717d7e4e8a131b61855a86f0d55aaf817
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review wangyugui/btrfs-fix-CHECK_INTEGRITY-warning-when-QUEUE_FLAG_WC/20211022-162718
        git checkout ced40ee717d7e4e8a131b61855a86f0d55aaf817
        # 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 errors (new ones prefixed by >>):

   fs/btrfs/disk-io.c: In function 'write_dev_flush':
>> fs/btrfs/disk-io.c:3981:24: error: unused variable 'q' [-Werror=unused-variable]
    3981 |  struct request_queue *q = bdev_get_queue(device->bdev);
         |                        ^
   cc1: all warnings being treated as errors


vim +/q +3981 fs/btrfs/disk-io.c

387125fc722a8e Chris Mason       2011-11-18  3974  
387125fc722a8e Chris Mason       2011-11-18  3975  /*
4fc6441aac7589 Anand Jain        2017-06-13  3976   * Submit a flush request to the device if it supports it. Error handling is
4fc6441aac7589 Anand Jain        2017-06-13  3977   * done in the waiting counterpart.
387125fc722a8e Chris Mason       2011-11-18  3978   */
4fc6441aac7589 Anand Jain        2017-06-13  3979  static void write_dev_flush(struct btrfs_device *device)
387125fc722a8e Chris Mason       2011-11-18  3980  {
c2a9c7ab475bc3 Anand Jain        2017-04-06 @3981  	struct request_queue *q = bdev_get_queue(device->bdev);
e0ae999414238a David Sterba      2017-06-06  3982  	struct bio *bio = device->flush_bio;
387125fc722a8e Chris Mason       2011-11-18  3983  
ced40ee717d7e4 wangyugui         2021-10-22  3984  	#ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
ced40ee717d7e4 wangyugui         2021-10-22  3985  	/*
ced40ee717d7e4 wangyugui         2021-10-22  3986  	* submit_bio(REQ_SYNC | REQ_PREFLUSH) can be skipped when !QUEUE_FLAG_WC.
ced40ee717d7e4 wangyugui         2021-10-22  3987  	* but btrfsic_submit_bio() != submit_bio() when CONFIG_BTRFS_FS_CHECK_INTEGRITY
ced40ee717d7e4 wangyugui         2021-10-22  3988  	*/
c2a9c7ab475bc3 Anand Jain        2017-04-06  3989  	if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
4fc6441aac7589 Anand Jain        2017-06-13  3990  		return;
ced40ee717d7e4 wangyugui         2021-10-22  3991  	#endif
387125fc722a8e Chris Mason       2011-11-18  3992  
e0ae999414238a David Sterba      2017-06-06  3993  	bio_reset(bio);
387125fc722a8e Chris Mason       2011-11-18  3994  	bio->bi_end_io = btrfs_end_empty_barrier;
74d46992e0d9de Christoph Hellwig 2017-08-23  3995  	bio_set_dev(bio, device->bdev);
8d91012528b3c9 Jan Kara          2017-05-02  3996  	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH;
387125fc722a8e Chris Mason       2011-11-18  3997  	init_completion(&device->flush_wait);
387125fc722a8e Chris Mason       2011-11-18  3998  	bio->bi_private = &device->flush_wait;
387125fc722a8e Chris Mason       2011-11-18  3999  
43a0111103af2d Lu Fengqi         2017-08-18  4000  	btrfsic_submit_bio(bio);
1c3063b6dbfa03 Anand Jain        2017-12-04  4001  	set_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state);
387125fc722a8e Chris Mason       2011-11-18  4002  }
387125fc722a8e Chris Mason       2011-11-18  4003  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Wang Yugui Nov. 2, 2021, 5:49 a.m. UTC | #3 
Hi,

This unused variable warning is already fixed in v2.

and some changelog is updated in this v3 too
https://patchwork.kernel.org/project/linux-btrfs/patch/20211027223254.8095-1-wangyugui@e16-tech.com/

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2021/11/02

> Hi wangyugui,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on kdave/for-next]
> [also build test ERROR on v5.15 next-20211101]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/wangyugui/btrfs-fix-CHECK_INTEGRITY-warning-when-QUEUE_FLAG_WC/20211022-162718
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: i386-allyesconfig (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/0day-ci/linux/commit/ced40ee717d7e4e8a131b61855a86f0d55aaf817
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review wangyugui/btrfs-fix-CHECK_INTEGRITY-warning-when-QUEUE_FLAG_WC/20211022-162718
>         git checkout ced40ee717d7e4e8a131b61855a86f0d55aaf817
>         # 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 errors (new ones prefixed by >>):
> 
>    fs/btrfs/disk-io.c: In function 'write_dev_flush':
> >> fs/btrfs/disk-io.c:3981:24: error: unused variable 'q' [-Werror=unused-variable]
>     3981 |  struct request_queue *q = bdev_get_queue(device->bdev);
>          |                        ^
>    cc1: all warnings being treated as errors
> 
> 
> vim +/q +3981 fs/btrfs/disk-io.c
> 
> 387125fc722a8e Chris Mason       2011-11-18  3974  
> 387125fc722a8e Chris Mason       2011-11-18  3975  /*
> 4fc6441aac7589 Anand Jain        2017-06-13  3976   * Submit a flush request to the device if it supports it. Error handling is
> 4fc6441aac7589 Anand Jain        2017-06-13  3977   * done in the waiting counterpart.
> 387125fc722a8e Chris Mason       2011-11-18  3978   */
> 4fc6441aac7589 Anand Jain        2017-06-13  3979  static void write_dev_flush(struct btrfs_device *device)
> 387125fc722a8e Chris Mason       2011-11-18  3980  {
> c2a9c7ab475bc3 Anand Jain        2017-04-06 @3981  	struct request_queue *q = bdev_get_queue(device->bdev);
> e0ae999414238a David Sterba      2017-06-06  3982  	struct bio *bio = device->flush_bio;
> 387125fc722a8e Chris Mason       2011-11-18  3983  
> ced40ee717d7e4 wangyugui         2021-10-22  3984  	#ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
> ced40ee717d7e4 wangyugui         2021-10-22  3985  	/*
> ced40ee717d7e4 wangyugui         2021-10-22  3986  	* submit_bio(REQ_SYNC | REQ_PREFLUSH) can be skipped when !QUEUE_FLAG_WC.
> ced40ee717d7e4 wangyugui         2021-10-22  3987  	* but btrfsic_submit_bio() != submit_bio() when CONFIG_BTRFS_FS_CHECK_INTEGRITY
> ced40ee717d7e4 wangyugui         2021-10-22  3988  	*/
> c2a9c7ab475bc3 Anand Jain        2017-04-06  3989  	if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
> 4fc6441aac7589 Anand Jain        2017-06-13  3990  		return;
> ced40ee717d7e4 wangyugui         2021-10-22  3991  	#endif
> 387125fc722a8e Chris Mason       2011-11-18  3992  
> e0ae999414238a David Sterba      2017-06-06  3993  	bio_reset(bio);
> 387125fc722a8e Chris Mason       2011-11-18  3994  	bio->bi_end_io = btrfs_end_empty_barrier;
> 74d46992e0d9de Christoph Hellwig 2017-08-23  3995  	bio_set_dev(bio, device->bdev);
> 8d91012528b3c9 Jan Kara          2017-05-02  3996  	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH;
> 387125fc722a8e Chris Mason       2011-11-18  3997  	init_completion(&device->flush_wait);
> 387125fc722a8e Chris Mason       2011-11-18  3998  	bio->bi_private = &device->flush_wait;
> 387125fc722a8e Chris Mason       2011-11-18  3999  
> 43a0111103af2d Lu Fengqi         2017-08-18  4000  	btrfsic_submit_bio(bio);
> 1c3063b6dbfa03 Anand Jain        2017-12-04  4001  	set_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state);
> 387125fc722a8e Chris Mason       2011-11-18  4002  }
> 387125fc722a8e Chris Mason       2011-11-18  4003  
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 355ea88..7b17357 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3971,8 +3971,14 @@  static void write_dev_flush(struct btrfs_device *device)
 	struct request_queue *q = bdev_get_queue(device->bdev);
 	struct bio *bio = device->flush_bio;
 
+	#ifndef CONFIG_BTRFS_FS_CHECK_INTEGRITY
+	/*
+	* submit_bio(REQ_SYNC | REQ_PREFLUSH) can be skipped when !QUEUE_FLAG_WC.
+	* but btrfsic_submit_bio() != submit_bio() when CONFIG_BTRFS_FS_CHECK_INTEGRITY
+	*/
 	if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags))
 		return;
+	#endif
 
 	bio_reset(bio);
 	bio->bi_end_io = btrfs_end_empty_barrier;