1. 26 Mar, 2018 10 commits
  2. 22 Mar, 2018 3 commits
    • Shawn Lin's avatar
      mmc: block: Delete gendisk before cleaning up the request queue · 57678e5a
      Shawn Lin authored
      dd if=/dev/urandom of=/dev/mmcblk1 bs=4k count=10000
      with a SD card hotplug during transfer reports a warning below
      introduced by commit a063057d ("block: Fix a race between
      request queue removal and the block cgroup controller"). So we
      should now remove the disk, partition and bdi sysfs attributes
      before cleaning up the request queue associated with the disk.
      
      [  410.331226] mmc1: card 59b4 removed
      [  410.348583] WARNING: CPU: 0 PID: 5 at block/blk-core.c:785
      blk_cleanup_queue+0x138/0x140
      [  410.349294] Modules linked in:
      [  410.349570] CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted
      4.16.0-rc6-next-20180321-00004-gc2ad6a7 #263
      [  410.350363] Hardware name: Excavator-RK3399 Board (DT)
      [  410.350819] Workqueue: events_freezable mmc_rescan
      [  410.351242] pstate: 60000005 (nZCv daif -PAN -UAO)
      [  410.351663] pc : blk_cleanup_queue+0x138/0x140
      [  410.352054] lr : blk_cleanup_queue+0xac/0x140
      [  410.352436] sp : ffff0000092cbb90
      [  410.352727] x29: ffff0000092cbb90 x28: 0000000000000000
      [  410.353195] x27: ffff8000f6f23030 x26: ffff00000904e610
      [  410.353662] x25: ffff8000f17cc808 x24: ffff8000f1038200
      [  410.354128] x23: 0000000000000060 x22: 0000000000000000
      [  410.354595] x21: ffff8000f11748d8 x20: ffff8000f1038200
      [  410.355061] x19: ffff8000f1174200 x18: 0000ffff936347d8
      [  410.355528] x17: 0000ffff935b93c0 x16: ffff0000081263f8
      [  410.355994] x15: 0000000000000000 x14: 0000000000000400
      [  410.356461] x13: 0000000000000001 x12: 0000000000000001
      [  410.356927] x11: 0000000000000040 x10: ffff8000f2400028
      [  410.357393] x9 : ffff8000f2400040 x8 : 0000000000000000
      [  410.357860] x7 : ffff8000f6f3a340 x6 : ffff8000f6f3a340
      [  410.358326] x5 : ffff8000f2400000 x4 : ffff8000f6f3a340
      [  410.358792] x3 : 0000000000000000 x2 : 39c1333e45670800
      [  410.359259] x1 : 0000000000000000 x0 : 0000000000000003
      [  410.359726] Call trace:
      [  410.359943]  blk_cleanup_queue+0x138/0x140
      [  410.360305]  mmc_cleanup_queue+0x2c/0x48
      [  410.360652]  mmc_blk_remove_req+0x1c/0x98
      [  410.361005]  mmc_blk_remove+0x180/0x1c0
      [  410.361343]  mmc_bus_remove+0x1c/0x28
      [  410.361670]  device_release_driver_internal+0x154/0x1f0
      [  410.362128]  device_release_driver+0x14/0x20
      [  410.362504]  bus_remove_device+0xc8/0x108
      [  410.362858]  device_del+0x120/0x350
      [  410.363167]  mmc_remove_card+0x5c/0xb8
      [  410.363498]  mmc_sd_detect+0x40/0x78
      [  410.363813]  mmc_rescan+0x19c/0x368
      [  410.364123]  process_one_work+0x1ac/0x318
      [  410.364477]  worker_thread+0x50/0x450
      [  410.364801]  kthread+0xf8/0x128
      [  410.365081]  ret_from_fork+0x10/0x18
      [  410.365395] ---[ end trace 268e87a46c28968c ]---
      Reviewed-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Signed-off-by: default avatarShawn Lin <shawn.lin@rock-chips.com>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      57678e5a
    • Mikulas Patocka's avatar
      Fix slab name "biovec-(1<<(21-12))" · bd5c4fac
      Mikulas Patocka authored
      I'm getting a slab named "biovec-(1<<(21-12))". It is caused by unintended
      expansion of the macro BIO_MAX_PAGES. This patch renames it to biovec-max.
      Signed-off-by: default avatarMikulas Patocka <mpatocka@redhat.com>
      Cc: stable@vger.kernel.org	# v4.14+
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      bd5c4fac
    • Mikulas Patocka's avatar
      block: use 32-bit blk_status_t on Alpha · 6e2fb221
      Mikulas Patocka authored
      Early alpha processors cannot write a single byte or word; they read 8
      bytes, modify the value in registers and write back 8 bytes.
      
      The type blk_status_t is defined as one byte, it is often written
      asynchronously by I/O completion routines, this asynchronous modification
      can corrupt content of nearby bytes if these nearby bytes can be written
      simultaneously by another CPU.
      
      - one example of such corruption is the structure dm_io where
        "blk_status_t status" is written by an asynchronous completion routine
        and "atomic_t io_count" is modified synchronously
      - another example is the structure dm_buffer where "unsigned hold_count"
        is modified synchronously from process context and "blk_status_t
        write_error" is modified asynchronously from bio completion routine
      
      This patch fixes the bug by changing the type blk_status_t to 32 bits if
      we are on Alpha and if we are compiling for a processor that doesn't have
      the byte-word-extension.
      Signed-off-by: default avatarMikulas Patocka <mpatocka@redhat.com>
      Cc: stable@vger.kernel.org	# 4.13+
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      6e2fb221
  3. 19 Mar, 2018 21 commits
    • Bart Van Assche's avatar
      block: Change a rcu_read_{lock,unlock}_sched() pair into rcu_read_{lock,unlock}() · 818e0fa2
      Bart Van Assche authored
      scsi_device_quiesce() uses synchronize_rcu() to guarantee that the
      effect of blk_set_preempt_only() will be visible for percpu_ref_tryget()
      calls that occur after the queue unfreeze by using the approach
      explained in https://lwn.net/Articles/573497/. The rcu read lock and
      unlock calls in blk_queue_enter() form a pair with the synchronize_rcu()
      call in scsi_device_quiesce(). Both scsi_device_quiesce() and
      blk_queue_enter() must either use regular RCU or RCU-sched.
      Since neither the RCU-protected code in blk_queue_enter() nor
      blk_queue_usage_counter_release() sleeps, regular RCU protection
      is sufficient. Note: scsi_device_quiesce() does not have to be
      modified since it already uses synchronize_rcu().
      Reported-by: default avatarTejun Heo <tj@kernel.org>
      Fixes: 3a0a5299 ("block, scsi: Make SCSI quiesce and resume work reliably")
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Acked-by: default avatarTejun Heo <tj@kernel.org>
      Cc: Tejun Heo <tj@kernel.org>
      Cc: Hannes Reinecke <hare@suse.com>
      Cc: Ming Lei <ming.lei@redhat.com>
      Cc: Christoph Hellwig <hch@lst.de>
      Cc: Johannes Thumshirn <jthumshirn@suse.de>
      Cc: Oleksandr Natalenko <oleksandr@natalenko.name>
      Cc: Martin Steigerwald <martin@lichtvoll.de>
      Cc: stable@vger.kernel.org # v4.15
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      818e0fa2
    • Bart Van Assche's avatar
      bcache: Fix a compiler warning in bcache_device_init() · 5f2b18ec
      Bart Van Assche authored
      Avoid that building with W=1 triggers the following compiler warning:
      
      drivers/md/bcache/super.c:776:20: warning: comparison is always false due to limited range of data type [-Wtype-limits]
            d->nr_stripes > SIZE_MAX / sizeof(atomic_t)) {
                          ^
      Reviewed-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      5f2b18ec
    • Bart Van Assche's avatar
      bcache: Reduce the number of sparse complaints about lock imbalances · 20d3a518
      Bart Van Assche authored
      Add more annotations for sparse to inform it about which functions do
      not have the same number of spin_lock() and spin_unlock() calls.
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      20d3a518
    • Bart Van Assche's avatar
      bcache: Suppress more warnings about set-but-not-used variables · 42361469
      Bart Van Assche authored
      This patch does not change any functionality.
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Reviewed-by: default avatarColy Li <colyli@suse.de>
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      42361469
    • Bart Van Assche's avatar
      bcache: Remove an unused variable · f0d38140
      Bart Van Assche authored
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      f0d38140
    • Bart Van Assche's avatar
      bcache: Fix kernel-doc warnings · 47344e33
      Bart Van Assche authored
      Avoid that building with W=1 triggers warnings about the kernel-doc
      headers.
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      47344e33
    • Bart Van Assche's avatar
      bcache: Annotate switch fall-through · 9dfbdec7
      Bart Van Assche authored
      This patch avoids that building with W=1 triggers complaints about
      switch fall-throughs.
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      9dfbdec7
    • Bart Van Assche's avatar
      bcache: Add __printf annotation to __bch_check_keys() · 4a4e4438
      Bart Van Assche authored
      Make it possible for the compiler to verify the consistency of the
      format string passed to __bch_check_keys() and the arguments that
      should be formatted according to that format string.
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      4a4e4438
    • Bart Van Assche's avatar
      bcache: Fix indentation · fd01991d
      Bart Van Assche authored
      This patch avoids that smatch complains about inconsistent indentation.
      Signed-off-by: default avatarBart Van Assche <bart.vanassche@wdc.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Reviewed-by: default avatarColy Li <colyli@suse.de>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      fd01991d
    • Coly Li's avatar
      bcache: add io_disable to struct cached_dev · c7b7bd07
      Coly Li authored
      If a bcache device is configured to writeback mode, current code does not
      handle write I/O errors on backing devices properly.
      
      In writeback mode, write request is written to cache device, and
      latter being flushed to backing device. If I/O failed when writing from
      cache device to the backing device, bcache code just ignores the error and
      upper layer code is NOT noticed that the backing device is broken.
      
      This patch tries to handle backing device failure like how the cache device
      failure is handled,
      - Add a error counter 'io_errors' and error limit 'error_limit' in struct
        cached_dev. Add another io_disable to struct cached_dev to disable I/Os
        on the problematic backing device.
      - When I/O error happens on backing device, increase io_errors counter. And
        if io_errors reaches error_limit, set cache_dev->io_disable to true, and
        stop the bcache device.
      
      The result is, if backing device is broken of disconnected, and I/O errors
      reach its error limit, backing device will be disabled and the associated
      bcache device will be removed from system.
      
      Changelog:
      v2: remove "bcache: " prefix in pr_error(), and use correct name string to
          print out bcache device gendisk name.
      v1: indeed this is new added in v2 patch set.
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarHannes Reinecke <hare@suse.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Michael Lyle <mlyle@lyle.org>
      Cc: Junhui Tang <tang.junhui@zte.com.cn>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      c7b7bd07
    • Coly Li's avatar
      bcache: add backing_request_endio() for bi_end_io · 27a40ab9
      Coly Li authored
      In order to catch I/O error of backing device, a separate bi_end_io
      call back is required. Then a per backing device counter can record I/O
      errors number and retire the backing device if the counter reaches a
      per backing device I/O error limit.
      
      This patch adds backing_request_endio() to bcache backing device I/O code
      path, this is a preparation for further complicated backing device failure
      handling. So far there is no real code logic change, I make this change a
      separate patch to make sure it is stable and reliable for further work.
      
      Changelog:
      v2: Fix code comments typo, remove a redundant bch_writeback_add() line
          added in v4 patch set.
      v1: indeed this is new added in this patch set.
      
      [mlyle: truncated commit subject]
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarHannes Reinecke <hare@suse.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Junhui Tang <tang.junhui@zte.com.cn>
      Cc: Michael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      27a40ab9
    • Chengguang Xu's avatar
      bcache: move closure debug file into debug directory · df2b9431
      Chengguang Xu authored
      In current code closure debug file is outside of debug directory
      and when unloading module there is lack of removing operation
      for closure debug file, so it will cause creating error when trying
      to reload  module.
      
      This patch move closure debug file into "bcache" debug direcory
      so that the file can get deleted properly.
      Signed-off-by: default avatarChengguang Xu <cgxu519@gmx.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Reviewed-by: default avatarTang Junhui <tang.junhui@zte.com.cn>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      df2b9431
    • Tang Junhui's avatar
      bcache: fix using of loop variable in memory shrink · ca71df31
      Tang Junhui authored
      In bch_mca_scan(), There are some confusion and logical error in the use of
      loop variables. In this patch, we clarify them as:
      1) nr: the number of btree nodes needs to scan, which will decrease after
      we scan a btree node, and should not be less than 0;
      2) i: the number of btree nodes have scanned, includes both
      btree_cache_freeable and btree_cache, which should not be bigger than
      btree_cache_used;
      3) freed: the number of btree nodes have freed.
      Signed-off-by: default avatarTang Junhui <tang.junhui@zte.com.cn>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      ca71df31
    • Tang Junhui's avatar
      bcache: fix error return value in memory shrink · f3641c3a
      Tang Junhui authored
      In bch_mca_scan(), the return value should not be the number of freed btree
      nodes, but the number of pages of freed btree nodes.
      Signed-off-by: default avatarTang Junhui <tang.junhui@zte.com.cn>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      f3641c3a
    • Tang Junhui's avatar
      bcache: fix incorrect sysfs output value of strip size · 688892b3
      Tang Junhui authored
      Stripe size is shown as zero when no strip in back end device:
      [root@ceph132 ~]# cat /sys/block/sdd/bcache/stripe_size
      0.0k
      
      Actually it should be 1T Bytes (1 << 31 sectors), but in sysfs
      interface, stripe_size was changed from sectors to bytes, and move
      9 bits left, so the 32 bits variable overflows.
      
      This patch change the variable to a 64 bits type before moving bits.
      Signed-off-by: default avatarTang Junhui <tang.junhui@zte.com.cn>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      688892b3
    • Tang Junhui's avatar
      bcache: fix inaccurate io state for detached bcache devices · bc082a55
      Tang Junhui authored
      When we run IO in a detached device,  and run iostat to shows IO status,
      normally it will show like bellow (Omitted some fields):
      Device: ... avgrq-sz avgqu-sz   await r_await w_await  svctm  %util
      sdd        ... 15.89     0.53    1.82    0.20    2.23   1.81  52.30
      bcache0    ... 15.89   115.42    0.00    0.00    0.00   2.40  69.60
      but after IO stopped, there are still very big avgqu-sz and %util
      values as bellow:
      Device: ... avgrq-sz avgqu-sz   await r_await w_await  svctm  %util
      bcache0   ...      0   5326.32    0.00    0.00    0.00   0.00 100.10
      
      The reason for this issue is that, only generic_start_io_acct() called
      and no generic_end_io_acct() called for detached device in
      cached_dev_make_request(). See the code:
      //start generic_start_io_acct()
      generic_start_io_acct(q, rw, bio_sectors(bio), &d->disk->part0);
      if (cached_dev_get(dc)) {
      	//will callback generic_end_io_acct()
      }
      else {
      	//will not call generic_end_io_acct()
      }
      
      This patch calls generic_end_io_acct() in the end of IO for detached
      devices, so we can show IO state correctly.
      
      (Modified to use GFP_NOIO in kzalloc() by Coly Li)
      
      Changelog:
      v2: fix typo.
      v1: the initial version.
      Signed-off-by: default avatarTang Junhui <tang.junhui@zte.com.cn>
      Reviewed-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarHannes Reinecke <hare@suse.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      bc082a55
    • Coly Li's avatar
      bcache: add stop_when_cache_set_failed option to backing device · 7e027ca4
      Coly Li authored
      When there are too many I/O errors on cache device, current bcache code
      will retire the whole cache set, and detach all bcache devices. But the
      detached bcache devices are not stopped, which is problematic when bcache
      is in writeback mode.
      
      If the retired cache set has dirty data of backing devices, continue
      writing to bcache device will write to backing device directly. If the
      LBA of write request has a dirty version cached on cache device, next time
      when the cache device is re-registered and backing device re-attached to
      it again, the stale dirty data on cache device will be written to backing
      device, and overwrite latest directly written data. This situation causes
      a quite data corruption.
      
      But we cannot simply stop all attached bcache devices when the cache set is
      broken or disconnected. For example, use bcache to accelerate performance
      of an email service. In such workload, if cache device is broken but no
      dirty data lost, keep the bcache device alive and permit email service
      continue to access user data might be a better solution for the cache
      device failure.
      
      Nix <nix@esperi.org.uk> points out the issue and provides the above example
      to explain why it might be necessary to not stop bcache device for broken
      cache device. Pavel Goran <via-bcache@pvgoran.name> provides a brilliant
      suggestion to provide "always" and "auto" options to per-cached device
      sysfs file stop_when_cache_set_failed. If cache set is retiring and the
      backing device has no dirty data on cache, it should be safe to keep the
      bcache device alive. In this case, if stop_when_cache_set_failed is set to
      "auto", the device failure handling code will not stop this bcache device
      and permit application to access the backing device with a unattached
      bcache device.
      
      Changelog:
      [mlyle: edited to not break string constants across lines]
      v3: fix typos pointed out by Nix.
      v2: change option values of stop_when_cache_set_failed from 1/0 to
          "auto"/"always".
      v1: initial version, stop_when_cache_set_failed can be 0 (not stop) or 1
          (always stop).
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Signed-off-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Nix <nix@esperi.org.uk>
      Cc: Pavel Goran <via-bcache@pvgoran.name>
      Cc: Junhui Tang <tang.junhui@zte.com.cn>
      Cc: Hannes Reinecke <hare@suse.com>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      7e027ca4
    • Coly Li's avatar
      bcache: add CACHE_SET_IO_DISABLE to struct cache_set flags · 771f393e
      Coly Li authored
      When too many I/Os failed on cache device, bch_cache_set_error() is called
      in the error handling code path to retire whole problematic cache set. If
      new I/O requests continue to come and take refcount dc->count, the cache
      set won't be retired immediately, this is a problem.
      
      Further more, there are several kernel thread and self-armed kernel work
      may still running after bch_cache_set_error() is called. It needs to wait
      quite a while for them to stop, or they won't stop at all. They also
      prevent the cache set from being retired.
      
      The solution in this patch is, to add per cache set flag to disable I/O
      request on this cache and all attached backing devices. Then new coming I/O
      requests can be rejected in *_make_request() before taking refcount, kernel
      threads and self-armed kernel worker can stop very fast when flags bit
      CACHE_SET_IO_DISABLE is set.
      
      Because bcache also do internal I/Os for writeback, garbage collection,
      bucket allocation, journaling, this kind of I/O should be disabled after
      bch_cache_set_error() is called. So closure_bio_submit() is modified to
      check whether CACHE_SET_IO_DISABLE is set on cache_set->flags. If set,
      closure_bio_submit() will set bio->bi_status to BLK_STS_IOERR and
      return, generic_make_request() won't be called.
      
      A sysfs interface is also added to set or clear CACHE_SET_IO_DISABLE bit
      from cache_set->flags, to disable or enable cache set I/O for debugging. It
      is helpful to trigger more corner case issues for failed cache device.
      
      Changelog
      v4, add wait_for_kthread_stop(), and call it before exits writeback and gc
          kernel threads.
      v3, change CACHE_SET_IO_DISABLE from 4 to 3, since it is bit index.
          remove "bcache: " prefix when printing out kernel message.
      v2, more changes by previous review,
      - Use CACHE_SET_IO_DISABLE of cache_set->flags, suggested by Junhui.
      - Check CACHE_SET_IO_DISABLE in bch_btree_gc() to stop a while-loop, this
        is reported and inspired from origal patch of Pavel Vazharov.
      v1, initial version.
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarHannes Reinecke <hare@suse.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Junhui Tang <tang.junhui@zte.com.cn>
      Cc: Michael Lyle <mlyle@lyle.org>
      Cc: Pavel Vazharov <freakpv@gmail.com>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      771f393e
    • Coly Li's avatar
      bcache: stop dc->writeback_rate_update properly · 3fd47bfe
      Coly Li authored
      struct delayed_work writeback_rate_update in struct cache_dev is a delayed
      worker to call function update_writeback_rate() in period (the interval is
      defined by dc->writeback_rate_update_seconds).
      
      When a metadate I/O error happens on cache device, bcache error handling
      routine bch_cache_set_error() will call bch_cache_set_unregister() to
      retire whole cache set. On the unregister code path, this delayed work is
      stopped by calling cancel_delayed_work_sync(&dc->writeback_rate_update).
      
      dc->writeback_rate_update is a special delayed work from others in bcache.
      In its routine update_writeback_rate(), this delayed work is re-armed
      itself. That means when cancel_delayed_work_sync() returns, this delayed
      work can still be executed after several seconds defined by
      dc->writeback_rate_update_seconds.
      
      The problem is, after cancel_delayed_work_sync() returns, the cache set
      unregister code path will continue and release memory of struct cache set.
      Then the delayed work is scheduled to run, __update_writeback_rate()
      will reference the already released cache_set memory, and trigger a NULL
      pointer deference fault.
      
      This patch introduces two more bcache device flags,
      - BCACHE_DEV_WB_RUNNING
        bit set:  bcache device is in writeback mode and running, it is OK for
                  dc->writeback_rate_update to re-arm itself.
        bit clear:bcache device is trying to stop dc->writeback_rate_update,
                  this delayed work should not re-arm itself and quit.
      - BCACHE_DEV_RATE_DW_RUNNING
        bit set:  routine update_writeback_rate() is executing.
        bit clear: routine update_writeback_rate() quits.
      
      This patch also adds a function cancel_writeback_rate_update_dwork() to
      wait for dc->writeback_rate_update quits before cancel it by calling
      cancel_delayed_work_sync(). In order to avoid a deadlock by unexpected
      quit dc->writeback_rate_update, after time_out seconds this function will
      give up and continue to call cancel_delayed_work_sync().
      
      And here I explain how this patch stops self re-armed delayed work properly
      with the above stuffs.
      
      update_writeback_rate() sets BCACHE_DEV_RATE_DW_RUNNING at its beginning
      and clears BCACHE_DEV_RATE_DW_RUNNING at its end. Before calling
      cancel_writeback_rate_update_dwork() clear flag BCACHE_DEV_WB_RUNNING.
      
      Before calling cancel_delayed_work_sync() wait utill flag
      BCACHE_DEV_RATE_DW_RUNNING is clear. So when calling
      cancel_delayed_work_sync(), dc->writeback_rate_update must be already re-
      armed, or quite by seeing BCACHE_DEV_WB_RUNNING cleared. In both cases
      delayed work routine update_writeback_rate() won't be executed after
      cancel_delayed_work_sync() returns.
      
      Inside update_writeback_rate() before calling schedule_delayed_work(), flag
      BCACHE_DEV_WB_RUNNING is checked before. If this flag is cleared, it means
      someone is about to stop the delayed work. Because flag
      BCACHE_DEV_RATE_DW_RUNNING is set already and cancel_delayed_work_sync()
      has to wait for this flag to be cleared, we don't need to worry about race
      condition here.
      
      If update_writeback_rate() is scheduled to run after checking
      BCACHE_DEV_RATE_DW_RUNNING and before calling cancel_delayed_work_sync()
      in cancel_writeback_rate_update_dwork(), it is also safe. Because at this
      moment BCACHE_DEV_WB_RUNNING is cleared with memory barrier. As I mentioned
      previously, update_writeback_rate() will see BCACHE_DEV_WB_RUNNING is clear
      and quit immediately.
      
      Because there are more dependences inside update_writeback_rate() to struct
      cache_set memory, dc->writeback_rate_update is not a simple self re-arm
      delayed work. After trying many different methods (e.g. hold dc->count, or
      use locks), this is the only way I can find which works to properly stop
      dc->writeback_rate_update delayed work.
      
      Changelog:
      v3: change values of BCACHE_DEV_WB_RUNNING and BCACHE_DEV_RATE_DW_RUNNING
          to bit index, for test_bit().
      v2: Try to fix the race issue which is pointed out by Junhui.
      v1: The initial version for review
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarJunhui Tang <tang.junhui@zte.com.cn>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Michael Lyle <mlyle@lyle.org>
      Cc: Hannes Reinecke <hare@suse.com>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      3fd47bfe
    • Coly Li's avatar
      bcache: quit dc->writeback_thread when BCACHE_DEV_DETACHING is set · fadd94e0
      Coly Li authored
      In patch "bcache: fix cached_dev->count usage for bch_cache_set_error()",
      cached_dev_get() is called when creating dc->writeback_thread, and
      cached_dev_put() is called when exiting dc->writeback_thread. This
      modification works well unless people detach the bcache device manually by
          'echo 1 > /sys/block/bcache<N>/bcache/detach'
      Because this sysfs interface only calls bch_cached_dev_detach() which wakes
      up dc->writeback_thread but does not stop it. The reason is, before patch
      "bcache: fix cached_dev->count usage for bch_cache_set_error()", inside
      bch_writeback_thread(), if cache is not dirty after writeback,
      cached_dev_put() will be called here. And in cached_dev_make_request() when
      a new write request makes cache from clean to dirty, cached_dev_get() will
      be called there. Since we don't operate dc->count in these locations,
      refcount d->count cannot be dropped after cache becomes clean, and
      cached_dev_detach_finish() won't be called to detach bcache device.
      
      This patch fixes the issue by checking whether BCACHE_DEV_DETACHING is
      set inside bch_writeback_thread(). If this bit is set and cache is clean
      (no existing writeback_keys), break the while-loop, call cached_dev_put()
      and quit the writeback thread.
      
      Please note if cache is still dirty, even BCACHE_DEV_DETACHING is set the
      writeback thread should continue to perform writeback, this is the original
      design of manually detach.
      
      It is safe to do the following check without locking, let me explain why,
      +	if (!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) &&
      +	    (!atomic_read(&dc->has_dirty) || !dc->writeback_running)) {
      
      If the kenrel thread does not sleep and continue to run due to conditions
      are not updated in time on the running CPU core, it just consumes more CPU
      cycles and has no hurt. This should-sleep-but-run is safe here. We just
      focus on the should-run-but-sleep condition, which means the writeback
      thread goes to sleep in mistake while it should continue to run.
      1, First of all, no matter the writeback thread is hung or not,
         kthread_stop() from cached_dev_detach_finish() will wake up it and
         terminate by making kthread_should_stop() return true. And in normal
         run time, bit on index BCACHE_DEV_DETACHING is always cleared, the
         condition
      	!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags)
         is always true and can be ignored as constant value.
      2, If one of the following conditions is true, the writeback thread should
         go to sleep,
         "!atomic_read(&dc->has_dirty)" or "!dc->writeback_running)"
         each of them independently controls the writeback thread should sleep or
         not, let's analyse them one by one.
      2.1 condition "!atomic_read(&dc->has_dirty)"
         If dc->has_dirty is set from 0 to 1 on another CPU core, bcache will
         call bch_writeback_queue() immediately or call bch_writeback_add() which
         indirectly calls bch_writeback_queue() too. In bch_writeback_queue(),
         wake_up_process(dc->writeback_thread) is called. It sets writeback
         thread's task state to TASK_RUNNING and following an implicit memory
         barrier, then tries to wake up the writeback thread.
         In writeback thread, its task state is set to TASK_INTERRUPTIBLE before
         doing the condition check. If other CPU core sets the TASK_RUNNING state
         after writeback thread setting TASK_INTERRUPTIBLE, the writeback thread
         will be scheduled to run very soon because its state is not
         TASK_INTERRUPTIBLE. If other CPU core sets the TASK_RUNNING state before
         writeback thread setting TASK_INTERRUPTIBLE, the implict memory barrier
         of wake_up_process() will make sure modification of dc->has_dirty on
         other CPU core is updated and observed on the CPU core of writeback
         thread. Therefore the condition check will correctly be false, and
         continue writeback code without sleeping.
      2.2 condition "!dc->writeback_running)"
         dc->writeback_running can be changed via sysfs file, every time it is
         modified, a following bch_writeback_queue() is alwasy called. So the
         change is always observed on the CPU core of writeback thread. If
         dc->writeback_running is changed from 0 to 1 on other CPU core, this
         condition check will observe the modification and allow writeback
         thread to continue to run without sleeping.
      Now we can see, even without a locking protection, multiple conditions
      check is safe here, no deadlock or process hang up will happen.
      
      I compose a separte patch because that patch "bcache: fix cached_dev->count
      usage for bch_cache_set_error()" already gets a "Reviewed-by:" from Hannes
      Reinecke. Also this fix is not trivial and good for a separate patch.
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Hannes Reinecke <hare@suse.com>
      Cc: Huijun Tang <tang.junhui@zte.com.cn>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      fadd94e0
    • Coly Li's avatar
      bcache: fix cached_dev->count usage for bch_cache_set_error() · 804f3c69
      Coly Li authored
      When bcache metadata I/O fails, bcache will call bch_cache_set_error()
      to retire the whole cache set. The expected behavior to retire a cache
      set is to unregister the cache set, and unregister all backing device
      attached to this cache set, then remove sysfs entries of the cache set
      and all attached backing devices, finally release memory of structs
      cache_set, cache, cached_dev and bcache_device.
      
      In my testing when journal I/O failure triggered by disconnected cache
      device, sometimes the cache set cannot be retired, and its sysfs
      entry /sys/fs/bcache/<uuid> still exits and the backing device also
      references it. This is not expected behavior.
      
      When metadata I/O failes, the call senquence to retire whole cache set is,
              bch_cache_set_error()
              bch_cache_set_unregister()
              bch_cache_set_stop()
              __cache_set_unregister()     <- called as callback by calling
                                              clousre_queue(&c->caching)
              cache_set_flush()            <- called as a callback when refcount
                                              of cache_set->caching is 0
              cache_set_free()             <- called as a callback when refcount
                                              of catch_set->cl is 0
              bch_cache_set_release()      <- called as a callback when refcount
                                              of catch_set->kobj is 0
      
      I find if kernel thread bch_writeback_thread() quits while-loop when
      kthread_should_stop() is true and searched_full_index is false, clousre
      callback cache_set_flush() set by continue_at() will never be called. The
      result is, bcache fails to retire whole cache set.
      
      cache_set_flush() will be called when refcount of closure c->caching is 0,
      and in function bcache_device_detach() refcount of closure c->caching is
      released to 0 by clousre_put(). In metadata error code path, function
      bcache_device_detach() is called by cached_dev_detach_finish(). This is a
      callback routine being called when cached_dev->count is 0. This refcount
      is decreased by cached_dev_put().
      
      The above dependence indicates, cache_set_flush() will be called when
      refcount of cache_set->cl is 0, and refcount of cache_set->cl to be 0
      when refcount of cache_dev->count is 0.
      
      The reason why sometimes cache_dev->count is not 0 (when metadata I/O fails
      and bch_cache_set_error() called) is, in bch_writeback_thread(), refcount
      of cache_dev is not decreased properly.
      
      In bch_writeback_thread(), cached_dev_put() is called only when
      searched_full_index is true and cached_dev->writeback_keys is empty, a.k.a
      there is no dirty data on cache. In most of run time it is correct, but
      when bch_writeback_thread() quits the while-loop while cache is still
      dirty, current code forget to call cached_dev_put() before this kernel
      thread exits. This is why sometimes cache_set_flush() is not executed and
      cache set fails to be retired.
      
      The reason to call cached_dev_put() in bch_writeback_rate() is, when the
      cache device changes from clean to dirty, cached_dev_get() is called, to
      make sure during writeback operatiions both backing and cache devices
      won't be released.
      
      Adding following code in bch_writeback_thread() does not work,
         static int bch_writeback_thread(void *arg)
              }
      
      +       if (atomic_read(&dc->has_dirty))
      +               cached_dev_put()
      +
              return 0;
       }
      because writeback kernel thread can be waken up and start via sysfs entry:
              echo 1 > /sys/block/bcache<N>/bcache/writeback_running
      It is difficult to check whether backing device is dirty without race and
      extra lock. So the above modification will introduce potential refcount
      underflow in some conditions.
      
      The correct fix is, to take cached dev refcount when creating the kernel
      thread, and put it before the kernel thread exits. Then bcache does not
      need to take a cached dev refcount when cache turns from clean to dirty,
      or to put a cached dev refcount when cache turns from ditry to clean. The
      writeback kernel thread is alwasy safe to reference data structure from
      cache set, cache and cached device (because a refcount of cache device is
      taken for it already), and no matter the kernel thread is stopped by I/O
      errors or system reboot, cached_dev->count can always be used correctly.
      
      The patch is simple, but understanding how it works is quite complicated.
      
      Changelog:
      v2: set dc->writeback_thread to NULL in this patch, as suggested by Hannes.
      v1: initial version for review.
      Signed-off-by: default avatarColy Li <colyli@suse.de>
      Reviewed-by: default avatarHannes Reinecke <hare@suse.com>
      Reviewed-by: default avatarMichael Lyle <mlyle@lyle.org>
      Cc: Michael Lyle <mlyle@lyle.org>
      Cc: Junhui Tang <tang.junhui@zte.com.cn>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      804f3c69
  4. 17 Mar, 2018 3 commits
  5. 16 Mar, 2018 2 commits
    • Joseph Qi's avatar
      blk-throttle: fix race between blkcg_bio_issue_check() and cgroup_rmdir() · 4c699480
      Joseph Qi authored
      We've triggered a WARNING in blk_throtl_bio() when throttling writeback
      io, which complains blkg->refcnt is already 0 when calling blkg_get(),
      and then kernel crashes with invalid page request.
      After investigating this issue, we've found it is caused by a race
      between blkcg_bio_issue_check() and cgroup_rmdir(), which is described
      below:
      
      writeback kworker               cgroup_rmdir
                                        cgroup_destroy_locked
                                          kill_css
                                            css_killed_ref_fn
                                              css_killed_work_fn
                                                offline_css
                                                  blkcg_css_offline
        blkcg_bio_issue_check
          rcu_read_lock
          blkg_lookup
                                                    spin_trylock(q->queue_lock)
                                                    blkg_destroy
                                                    spin_unlock(q->queue_lock)
          blk_throtl_bio
          spin_lock_irq(q->queue_lock)
          ...
          spin_unlock_irq(q->queue_lock)
        rcu_read_unlock
      
      Since rcu can only prevent blkg from releasing when it is being used,
      the blkg->refcnt can be decreased to 0 during blkg_destroy() and schedule
      blkg release.
      Then trying to blkg_get() in blk_throtl_bio() will complains the WARNING.
      And then the corresponding blkg_put() will schedule blkg release again,
      which result in double free.
      This race is introduced by commit ae118896 ("blkcg: consolidate blkg
      creation in blkcg_bio_issue_check()"). Before this commit, it will
      lookup first and then try to lookup/create again with queue_lock. Since
      revive this logic is a bit drastic, so fix it by only offlining pd during
      blkcg_css_offline(), and move the rest destruction (especially
      blkg_put()) into blkcg_css_free(), which should be the right way as
      discussed.
      
      Fixes: ae118896 ("blkcg: consolidate blkg creation in blkcg_bio_issue_check()")
      Reported-by: default avatarJiufei Xue <jiufei.xue@linux.alibaba.com>
      Signed-off-by: default avatarJoseph Qi <joseph.qi@linux.alibaba.com>
      Acked-by: default avatarTejun Heo <tj@kernel.org>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      4c699480
    • Jonas Rabenstein's avatar
      block: sed-opal: fix u64 short atom length · 5f990d31
      Jonas Rabenstein authored
      The length must be given as bytes and not as 4 bit tuples.
      Reviewed-by: default avatarScott Bauer <scott.bauer@intel.com>
      Signed-off-by: default avatarJonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      5f990d31
  6. 13 Mar, 2018 1 commit
    • Christoph Hellwig's avatar
      bsg: split handling of SCSI CDBs vs transport requeues · 17cb960f
      Christoph Hellwig authored
      The current BSG design tries to shoe-horn the transport-specific
      passthrough commands into the overall framework for SCSI passthrough
      requests.  This has a couple problems:
      
       - each passthrough queue has to set the QUEUE_FLAG_SCSI_PASSTHROUGH flag
         despite not dealing with SCSI commands at all.  Because of that these
         queues could also incorrectly accept SCSI commands from in-kernel
         users or through the legacy SCSI_IOCTL_SEND_COMMAND ioctl.
       - the real SCSI bsg queues also incorrectly accept bsg requests of the
         BSG_SUB_PROTOCOL_SCSI_TRANSPORT type
       - the bsg transport code is almost unredable because it tries to reuse
         different SCSI concepts for its own purpose.
      
      This patch instead adds a new bsg_ops structure to handle the two cases
      differently, and thus solves all of the above problems.  Another side
      effect is that the bsg-lib queues also don't need to embedd a
      struct scsi_request anymore.
      Signed-off-by: default avatarChristoph Hellwig <hch@lst.de>
      Reviewed-by: default avatarHannes Reinecke <hare@suse.com>
      Reviewed-by: default avatarJohannes Thumshirn <jthumshirn@suse.de>
      Signed-off-by: default avatarJens Axboe <axboe@kernel.dk>
      17cb960f