| Message ID | 20201105065008.401112-1-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: Set mq device as blocking with zoned mode | expand |
On Thu, Nov 05, 2020 at 03:50:08PM +0900, Damien Le Moal wrote: > Commit aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") changed zone > locking to using the potentially sleeping wait_on_bit_io() function. A > zoned null_blk device must thus be marked as blocking to avoid calls to > queue_rq() from invalid contexts triggering might_sleep() warnings. That's going to have a fair amount of overhead. Can't you change the locking to avoid blocking?
On 2020/11/05 16:54, Christoph Hellwig wrote: > On Thu, Nov 05, 2020 at 03:50:08PM +0900, Damien Le Moal wrote: >> Commit aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") changed zone >> locking to using the potentially sleeping wait_on_bit_io() function. A >> zoned null_blk device must thus be marked as blocking to avoid calls to >> queue_rq() from invalid contexts triggering might_sleep() warnings. > > That's going to have a fair amount of overhead. Can't you change the > locking to avoid blocking? > I know, but the GFP_NOIO allocation with memory backing prevents any sort of spinlock. So unless we change that to GFP_NOWAIT I do not see a way out of it. I am fine with GFP_NOWAIT, but not sure if that is not going to break some test setups out there. The other solution would be to "do writes" (memory allocation and copy) first, regardless of zone state, wp etc, then do the zone checking under a spinlock, with that eventually failing the IO despite the copy already done. But for a test device, I think that is acceptable. Any other idea ?
On 2020/11/05 16:59, Damien Le Moal wrote: > On 2020/11/05 16:54, Christoph Hellwig wrote: >> On Thu, Nov 05, 2020 at 03:50:08PM +0900, Damien Le Moal wrote: >>> Commit aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") changed zone >>> locking to using the potentially sleeping wait_on_bit_io() function. A >>> zoned null_blk device must thus be marked as blocking to avoid calls to >>> queue_rq() from invalid contexts triggering might_sleep() warnings. >> >> That's going to have a fair amount of overhead. Can't you change the >> locking to avoid blocking? >> > > I know, but the GFP_NOIO allocation with memory backing prevents any sort of > spinlock. So unless we change that to GFP_NOWAIT I do not see a way out of it. > I am fine with GFP_NOWAIT, but not sure if that is not going to break some test > setups out there. > > The other solution would be to "do writes" (memory allocation and copy) first, > regardless of zone state, wp etc, then do the zone checking under a spinlock, > with that eventually failing the IO despite the copy already done. But for a > test device, I think that is acceptable. This idea is garbage. That obviously would break data if invalid IOs are requested (e.g. a non aligned write). But doing the data copy (which may trigger allocation) *after* the locked zone checks may actually work fine. Still scratching my head thinking potential problems. > > Any other idea ? > >
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c index 4685ea401d5b..9b4e22c8cc78 100644 --- a/drivers/block/null_blk_main.c +++ b/drivers/block/null_blk_main.c @@ -1736,10 +1736,11 @@ static int null_validate_conf(struct nullb_device *dev) dev->queue_mode = min_t(unsigned int, dev->queue_mode, NULL_Q_MQ); dev->irqmode = min_t(unsigned int, dev->irqmode, NULL_IRQ_TIMER); - /* Do memory allocation, so set blocking */ - if (dev->memory_backed) + /* Memory allocation and zone handling may sleep, so set blocking */ + if (dev->memory_backed || dev->zoned) dev->blocking = true; - else /* cache is meaningless */ + /* Cache is meaningless without memory backing */ + if (!dev->memory_backed) dev->cache_size = 0; dev->cache_size = min_t(unsigned long, ULONG_MAX / 1024 / 1024, dev->cache_size);
Commit aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") changed zone locking to using the potentially sleeping wait_on_bit_io() function. A zoned null_blk device must thus be marked as blocking to avoid calls to queue_rq() from invalid contexts triggering might_sleep() warnings. Reported-by: kernel test robot <lkp@intel.com> Fixes: aa1c09cb65e2 ("null_blk: Fix locking in zoned mode") Cc: stable@vger.kernel.org Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- drivers/block/null_blk_main.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)