* Re: [PATCHv6 15/37] thp: do not threat slab pages as huge in hpage_{nr_pages,size,mask}
From: Matthew Wilcox @ 2017-02-10 22:13 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Theodore Ts'o, Andreas Dilger, Jan Kara, Andrew Morton,
Alexander Viro, Hugh Dickins, Andrea Arcangeli, Dave Hansen,
Vlastimil Babka, Ross Zwisler, linux-ext4, linux-fsdevel,
linux-kernel, linux-mm, linux-block
In-Reply-To: <20170126115819.58875-16-kirill.shutemov@linux.intel.com>
On Thu, Jan 26, 2017 at 02:57:57PM +0300, Kirill A. Shutemov wrote:
> Slab pages can be compound, but we shouldn't threat them as THP for
> pupose of hpage_* helpers, otherwise it would lead to confusing results.
>
> For instance, ext4 uses slab pages for journal pages and we shouldn't
> confuse them with THPs. The easiest way is to exclude them in hpage_*
> helpers.
Well ... I think we should just deal with compound pages instead of just
huge or regular. So I'm deferring comment on this patch.
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
> include/linux/huge_mm.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index e5c9c26d2439..5e6c408f5b47 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -137,21 +137,21 @@ static inline spinlock_t *pmd_trans_huge_lock(pmd_t *pmd,
> }
> static inline int hpage_nr_pages(struct page *page)
> {
> - if (unlikely(PageTransHuge(page)))
> + if (unlikely(!PageSlab(page) && PageTransHuge(page)))
> return HPAGE_PMD_NR;
> return 1;
> }
>
> static inline int hpage_size(struct page *page)
> {
> - if (unlikely(PageTransHuge(page)))
> + if (unlikely(!PageSlab(page) && PageTransHuge(page)))
> return HPAGE_PMD_SIZE;
> return PAGE_SIZE;
> }
>
> static inline unsigned long hpage_mask(struct page *page)
> {
> - if (unlikely(PageTransHuge(page)))
> + if (unlikely(!PageSlab(page) && PageTransHuge(page)))
> return HPAGE_PMD_MASK;
> return PAGE_MASK;
> }
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v1 0/5] md: use bio_clone_fast()
From: Shaohua Li @ 2017-02-11 0:38 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1486724177-14817-1-git-send-email-tom.leiming@gmail.com>
On Fri, Feb 10, 2017 at 06:56:12PM +0800, Ming Lei wrote:
> Hi,
>
> This patches replaces bio_clone() with bio_fast_clone() in
> bio_clone_mddev() because:
>
> 1) bio_clone_mddev() is used in raid normal I/O and isn't in
> resync I/O path, and all the direct access to bvec table in
> raid happens on resync I/O only except for write behind of raid1.
> Write behind is treated specially, so the replacement is safe.
>
> 2) for write behind, bio_clone() is kept, but this patchset
> introduces bio_clone_bioset_partial() to just clone one specific
> bvecs range instead of whole table. Then write behind is improved
> too.
Thanks! this patch set looks good to me.
Jens,
can you look at the first patch? If it's ok, I'll carry it in my tree.
Thanks,
Shaohua
> V1:
> 1) don't introduce bio_clone_slow_mddev_partial()
> 2) return failure if mddev->bio_set can't be created
> 3) remove check in bio_clone_mddev() as suggested by
> Christoph Hellwig.
> 4) rename bio_clone_mddev() as bio_clone_fast_mddev()
>
>
> Ming Lei (5):
> block: introduce bio_clone_bioset_partial()
> md/raid1: use bio_clone_bioset_partial() in case of write behind
> md: fail if mddev->bio_set can't be created
> md: remove unnecessary check on mddev
> md: fast clone bio in bio_clone_mddev()
>
> block/bio.c | 61 +++++++++++++++++++++++++++++++++++++++++------------
> drivers/md/faulty.c | 2 +-
> drivers/md/md.c | 14 ++++++------
> drivers/md/md.h | 4 ++--
> drivers/md/raid1.c | 26 ++++++++++++++++-------
> drivers/md/raid10.c | 11 +++++-----
> drivers/md/raid5.c | 4 ++--
> include/linux/bio.h | 11 ++++++++--
> 8 files changed, 92 insertions(+), 41 deletions(-)
>
> --
> 2.7.4
>
> Thanks,
> Ming
^ permalink raw reply
* [PATCH] block/loop: fix race between I/O and set_status
From: Ming Lei @ 2017-02-11 3:40 UTC (permalink / raw)
To: Jens Axboe, linux-kernel
Cc: linux-block, Christoph Hellwig, Dmitry Vyukov, Ming Lei, stable
Inside set_status, transfer need to setup again, so
we have to drain IO before the transition, otherwise
oops may be triggered like the following:
divide error: 0000 [#1] SMP KASAN
CPU: 0 PID: 2935 Comm: loop7 Not tainted 4.10.0-rc7+ #213
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
01/01/2011
task: ffff88006ba1e840 task.stack: ffff880067338000
RIP: 0010:transfer_xor+0x1d1/0x440 drivers/block/loop.c:110
RSP: 0018:ffff88006733f108 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff8800688d7000 RCX: 0000000000000059
RDX: 0000000000000000 RSI: 1ffff1000d743f43 RDI: ffff880068891c08
RBP: ffff88006733f160 R08: ffff8800688d7001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8800688d7000
R13: ffff880067b7d000 R14: dffffc0000000000 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff88006d000000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000006c17e0 CR3: 0000000066e3b000 CR4: 00000000001406f0
Call Trace:
lo_do_transfer drivers/block/loop.c:251 [inline]
lo_read_transfer drivers/block/loop.c:392 [inline]
do_req_filebacked drivers/block/loop.c:541 [inline]
loop_handle_cmd drivers/block/loop.c:1677 [inline]
loop_queue_work+0xda0/0x49b0 drivers/block/loop.c:1689
kthread_worker_fn+0x4c3/0xa30 kernel/kthread.c:630
kthread+0x326/0x3f0 kernel/kthread.c:227
ret_from_fork+0x31/0x40 arch/x86/entry/entry_64.S:430
Code: 03 83 e2 07 41 29 df 42 0f b6 04 30 4d 8d 44 24 01 38 d0 7f 08
84 c0 0f 85 62 02 00 00 44 89 f8 41 0f b6 48 ff 25 ff 01 00 00 99 <f7>
7d c8 48 63 d2 48 03 55 d0 48 89 d0 48 89 d7 48 c1 e8 03 83
RIP: transfer_xor+0x1d1/0x440 drivers/block/loop.c:110 RSP:
ffff88006733f108
---[ end trace 0166f7bd3b0c0933 ]---
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/block/loop.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ed5259510857..4b52a1690329 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1097,9 +1097,12 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
return -EINVAL;
+ /* I/O need to be drained during transfer transition */
+ blk_mq_freeze_queue(lo->lo_queue);
+
err = loop_release_xfer(lo);
if (err)
- return err;
+ goto exit;
if (info->lo_encrypt_type) {
unsigned int type = info->lo_encrypt_type;
@@ -1114,12 +1117,14 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
err = loop_init_xfer(lo, xfer, info);
if (err)
- return err;
+ goto exit;
if (lo->lo_offset != info->lo_offset ||
lo->lo_sizelimit != info->lo_sizelimit)
- if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit))
- return -EFBIG;
+ if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
+ err = -EFBIG;
+ goto exit;
+ }
loop_config_discard(lo);
@@ -1156,7 +1161,9 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
/* update dio if lo_offset or transfer is changed */
__loop_update_dio(lo, lo->use_dio);
- return 0;
+ exit:
+ blk_mq_unfreeze_queue(lo->lo_queue);
+ return err;
}
static int
--
2.7.4
^ permalink raw reply related
* Re: [Nbd] [PATCH] nbd: set the logical and physical blocksize properly
From: Wouter Verhelst @ 2017-02-11 11:43 UTC (permalink / raw)
To: Josef Bacik
Cc: Alex Bligh, axboe, linux-block, kernel-team,
nbd-general@lists.sourceforge.net
In-Reply-To: <1486763262.4094.12.camel@fb.com>
On Fri, Feb 10, 2017 at 04:47:42PM -0500, Josef Bacik wrote:
> On Fri, 2017-02-10 at 21:07 +0100, Alex Bligh wrote:
> > >
> > > On 10 Feb 2017, at 19:06, Josef Bacik <jbacik@fb.com> wrote:
> > >
> > > We noticed when trying to do O_DIRECT to an export on the server
> > > side
> > > that we were getting requests smaller than the 4k sectorsize of the
> > > device.��This is because the client isn't setting the logical and
> > > physical blocksizes properly for the underlying device.��Fix this
> > > up by
> > > setting the queue blocksizes and then calling bd_set_size.
> > Interesting. Some input into the info extension (re blocksizes) would
> > definitely be appreciated.
> >
>
> What do you mean? �Right now the client is just calling�NBD_SET_BLKSIZE
> with 4k blocksize since all of our devices are 4k drives. �Thanks,
He's talking about
<https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md#block-size-constraints>,
which is a protocol extension that hasn't been implemented yet but would
be relevant to this patch.
--
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
people in the world who think they really understand all of its rules,
and pretty much all of them are just lying to themselves too.
-- #debian-devel, OFTC, 2016-02-12
^ permalink raw reply
* RE: [PATCH 00/16] multiqueue for MMC/SD third try
From: Avri Altman @ 2017-02-11 13:03 UTC (permalink / raw)
To: Linus Walleij, linux-mmc@vger.kernel.org, Ulf Hansson,
Adrian Hunter, Paolo Valente
Cc: Chunyan Zhang, Baolin Wang, linux-block@vger.kernel.org,
Jens Axboe, Christoph Hellwig, Arnd Bergmann
In-Reply-To: <20170209153403.9730-1-linus.walleij@linaro.org>
>
> The iozone results seem a bit consistent and all values seem to be noisy and
> not say much. I don't know why really, maybe the test is simply not relevant,
> the tests don't seem to be significantly affected by any of the patches, so
> let's focus on the dd and find tests.
Maybe use a more selective testing mode instead of -az.
Also maybe you want to clear the cache between the sequential and random tests:
#sync�
#echo 3 > /proc/sys/vm/drop_caches�
#sync�
It helps to obtain a more robust results.
> What immediately jumps out at you is that linear read/writes perform just as
> nicely or actually better with MQ than with the old block layer.
How come 22.7MB/s before vs. 22.1MB/s after is better? or did I misunderstand the output?
Also as dd is probably using the buffer cache, unlike the iozone test in which you properly used -I
for direct mode to isolate the blk-mq effect - does it really say much?
^ permalink raw reply
* Re: [lkp-robot] [scsi, block] 0dba1314d4: WARNING:at_fs/sysfs/dir.c:#sysfs_warn_dup
From: James Bottomley @ 2017-02-11 16:07 UTC (permalink / raw)
To: Jens Axboe, Dan Williams, Christoph Hellwig
Cc: kernel test robot, Bart Van Assche, Martin K. Petersen, Jan Kara,
Omar Sandoval, Omar Sandoval, LKML, Jens Axboe, LKP, linux-scsi,
linux-block
In-Reply-To: <01c338e2-2a30-610d-b7fa-00cb3ce2cf86@fb.com>
On Mon, 2017-02-06 at 21:09 -0700, Jens Axboe wrote:
> On 02/06/2017 05:14 PM, James Bottomley wrote:
> > On Sun, 2017-02-05 at 21:13 -0800, Dan Williams wrote:
> > > On Sun, Feb 5, 2017 at 1:13 AM, Christoph Hellwig <hch@lst.de>
> > > wrote:
> > > > Dan,
> > > >
> > > > can you please quote your emails? I can't find any content
> > > > inbetween all these quotes.
> > >
> > > Sorry, I'm using gmail, but I'll switch to attaching the logs.
> > >
> > > So with help from Xiaolong I was able to reproduce this, and it
> > > does
> > > not appear to be a regression. We simply change the failure
> > > output of
> > > an existing bug. Attached is a log of the same test on v4.10-rc7
> > > (i.e. without the recent block/scsi fixes), and it shows sda
> > > being
> > > registered twice.
> > >
> > > "[ 6.647077] kobject (d5078ca4): tried to init an initialized
> > > object, something is seriously wrong."
> > >
> > > The change that "scsi, block: fix duplicate bdi name registration
> > > crashes" makes is to properly try to register sdb since the sda
> > > devt
> > > is still alive. However that's not a fix because we've managed to
> > > call blk_register_queue() twice on the same queue.
> >
> > OK, time to involve others: linux-scsi and linux-block cc'd and
> > I've
> > inserted the log below.
> >
> > James
> >
> > ---
> >
> > [ 5.969672] scsi host0: scsi_debug: version 1.86 [20160430]
> > [ 5.969672] dev_size_mb=8, opts=0x0, submit_queues=1,
> > statistics=0
> > [ 5.971895] scsi 0:0:0:0: Direct-Access Linux scsi_debug
> > 0186 PQ: 0 ANSI: 7
> > [ 6.006983] sd 0:0:0:0: [sda] 16384 512-byte logical blocks:
> > (8.39 MB/8.00 MiB)
> > [ 6.026965] sd 0:0:0:0: [sda] Write Protect is off
> > [ 6.027870] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
> > [ 6.066962] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
> > enabled, supports DPO and FUA
> > [ 6.486962] sd 0:0:0:0: [sda] Attached SCSI disk
> > [ 6.488377] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> > [ 6.489455] sd 0:0:0:0: Attached scsi generic sg0 type 0
> > [ 6.526982] sd 0:0:0:0: [sda] 16384 512-byte logical blocks:
> > (8.39 MB/8.00 MiB)
> > [ 6.546964] sd 0:0:0:0: [sda] Write Protect is off
> > [ 6.547873] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
> > [ 6.586963] sd 0:0:0:0: [sda] Write cache: enabled, read cache:
> > enabled, supports DPO and FUA
> > [ 6.647077] kobject (d5078ca4): tried to init an initialized
> > object, something is seriously wrong.
>
> So sda is probed twice, and hilarity ensues when we try to register
> it twice. I can't reproduce this, using scsi_debug and with
> scsi_async enabled.
Actually, when you look closely, it's not a double add; it's an
add/remove/add. You can see this from
[ 6.488377] sd 0:0:0:0: [sda] Synchronizing SCSI cache
That's from sd_shutdown() as the driver is removing. It looks like
something with the config caused the built in SCSI debug to do this
(not sure why), but since the stack trace is in block, I think the bug
is in the remove path: something didn't release the mq object
correctly.
James
^ permalink raw reply
* Re: [Nbd] [PATCH] nbd: set the logical and physical blocksize properly
From: Alex Bligh @ 2017-02-11 16:13 UTC (permalink / raw)
To: Wouter Verhelst
Cc: Alex Bligh, Josef Bacik, axboe, linux-block, Kernel Team,
nbd-general@lists.sourceforge.net
In-Reply-To: <20170211114356.qr2atsx4ewcjfbes@grep.be>
> On 11 Feb 2017, at 12:43, Wouter Verhelst <w@uter.be> wrote:
>=20
>>> Interesting. Some input into the info extension (re blocksizes) =
would
>>> definitely be appreciated.
>>>=20
>>=20
>> What do you mean? Right now the client is just calling =
NBD_SET_BLKSIZE
>> with 4k blocksize since all of our devices are 4k drives. Thanks,
>=20
> He's talking about
> =
<https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.m=
d#block-size-constraints>,
> which is a protocol extension that hasn't been implemented yet but =
would
> be relevant to this patch.
Indeed. Specifically, review of the patch from a kernel perspective
would be useful.
--=20
Alex Bligh
^ permalink raw reply
* Re: [PATCH 01/10] block: Move bdev_unhash_inode() after invalidate_partition()
From: Tejun Heo @ 2017-02-12 3:58 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-2-jack@suse.cz>
On Thu, Feb 09, 2017 at 01:44:24PM +0100, Jan Kara wrote:
> Move bdev_unhash_inode() after invalidate_partition() as
> invalidate_partition() looks up bdev and will unnecessarily recreate it
> if bdev_unhash_inode() destroyed it. Also use part_devt() when calling
> bdev_unhash_inode() instead of manually creating the device number.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
> @@ -648,9 +648,8 @@ void del_gendisk(struct gendisk *disk)
> disk_part_iter_init(&piter, disk,
> DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
> while ((part = disk_part_iter_next(&piter))) {
> - bdev_unhash_inode(MKDEV(disk->major,
> - disk->first_minor + part->partno));
> invalidate_partition(disk, part->partno);
> + bdev_unhash_inode(part_devt(part));
> delete_partition(disk, part->partno);
So, before this patch, invalidate_partition() would have operated on a
newly created inode and thus wouldn't have actually invalidated the
existing bdev / mapping, right?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 02/10] block: Unhash also block device inode for the whole device
From: Tejun Heo @ 2017-02-12 4:16 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-3-jack@suse.cz>
On Thu, Feb 09, 2017 at 01:44:25PM +0100, Jan Kara wrote:
> Iteration over partitions in del_gendisk() omits part0. Add
> bdev_unhash_inode() call for the whole device. Otherwise if the device
> number gets reused, bdev inode will be still associated with the old
> (stale) bdi.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 03/10] block: Revalidate i_bdev reference in bd_aquire()
From: Tejun Heo @ 2017-02-12 4:22 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209155417.GA15818@quack2.suse.cz>
On Thu, Feb 09, 2017 at 04:54:17PM +0100, Jan Kara wrote:
> From aaf612333753b948a96aebe4a2f8066ed45ef164 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Thu, 9 Feb 2017 12:16:30 +0100
> Subject: [PATCH 03/10] block: Revalidate i_bdev reference in bd_aquire()
>
> When a device gets removed, block device inode unhashed so that it is not
> used anymore (bdget() will not find it anymore). Later when a new device
> gets created with the same device number, we create new block device
> inode. However there may be file system device inodes whose i_bdev still
> points to the original block device inode and thus we get two active
> block device inodes for the same device. They will share the same
> gendisk so the only visible differences will be that page caches will
> not be coherent and BDIs will be different (the old block device inode
> still points to unregistered BDI).
>
> Fix the problem by checking in bd_acquire() whether i_bdev still points
> to active block device inode and re-lookup the block device if not. That
> way any open of a block device happening after the old device has been
> removed will get correct block device inode.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 04/10] block: Move bdi_unregister() to del_gendisk()
From: Tejun Heo @ 2017-02-12 4:31 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-5-jack@suse.cz>
On Thu, Feb 09, 2017 at 01:44:27PM +0100, Jan Kara wrote:
> Commit 6cd18e711dd8 "block: destroy bdi before blockdev is
> unregistered." moved bdi unregistration (at that time through
> bdi_destroy()) from blk_release_queue() to blk_cleanup_queue() because
> it needs to happen before blk_unregister_region() call in del_gendisk()
> for MD. As much as it is fine for device registration / unregistration
> purposes, it does not fit our needs wrt writeback code. For those we
> will need bdi_unregister() to happen after bdev_unhash_inode() so that
> we are sure bdev inode is destroyed or soon to be destroyed (as soon as
> last inode reference is dropped and nobody should be holding bdev inode
> reference for long at this point) because bdi_unregister() may block
> waiting for bdev's inode i_wb reference to be dropped and that happens
> only once bdev inode gets destroyed.
>
> Also SCSI will free up the device number from sd_remove() called through
> a maze of callbacks from device_del() in __scsi_remove_device() before
> blk_cleanup_queue() and thus similar races as described in 6cd18e711dd8
> can happen for SCSI as well as reported by Omar [1]. Moving
> bdi_unregister() to del_gendisk() fixes these problems as well since
> del_gendisk() gets called from sd_remove() before freeing the device
> number.
>
> This also makes device_add_disk() (calling bdi_register_owner()) more
> symmetric with del_gendisk().
>
> [1] http://marc.info/?l=linux-block&m=148554717109098&w=2
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 05/10] writeback: Generalize and standardize I_SYNC waiting function
From: Tejun Heo @ 2017-02-12 4:32 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-6-jack@suse.cz>
On Thu, Feb 09, 2017 at 01:44:28PM +0100, Jan Kara wrote:
> __inode_wait_for_writeback() waits for I_SYNC on inode to get cleared.
> There's nothing specific regarting I_SYNC for that function. Generalize
> it so that we can use it also for I_WB_SWITCH bit. Also the function
> uses __wait_on_bit() unnecessarily. Switch it to wait_on_bit() to remove
> some code.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 08/10] block: Fix oops in locked_inode_to_wb_and_lock_list()
From: Tejun Heo @ 2017-02-12 4:40 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-9-jack@suse.cz>
Hello, Jan.
On Thu, Feb 09, 2017 at 01:44:31PM +0100, Jan Kara wrote:
> When block device is closed, we call inode_detach_wb() in __blkdev_put()
> which sets inode->i_wb to NULL. That is contrary to expectations that
> inode->i_wb stays valid once set during the whole inode's lifetime and
> leads to oops in wb_get() in locked_inode_to_wb_and_lock_list() because
> inode_to_wb() returned NULL.
>
> The reason why we called inode_detach_wb() is not valid anymore though.
> BDI is guaranteed to stay along until we call bdi_put() from
> bdev_evict_inode() so we can postpone calling inode_detach_wb() to that
> moment. A complication is that i_wb can point to non-root wb_writeback
> structure and in that case we do need to clean it up as bdi_unregister()
> blocks waiting for all non-root wb_writeback references to get dropped.
> Thus this i_wb reference could block device removal e.g. from
> __scsi_remove_device() (which indirectly ends up calling
> bdi_unregister()). We cannot rely on block device inode to go away soon
> (and thus i_wb reference to get dropped) as the device may got
> hot-removed e.g. under a mounted filesystem. We deal with these issues
> by switching block device inode from non-root wb_writeback structure to
> bdi->wb when needed. Since this is rather expensive (requires
> synchronize_rcu()) we do the switching only in del_gendisk() when we
> know the device is going away.
So, the only reason cgwb_bdi_destroy() is synchronous is because bdi
destruction was synchronous. Now that bdi is properly reference
counted and can be decoupled from gendisk / q destruction, I can't
think of a reason to keep cgwb destruction synchronous. Switching
wb's on destruction is kinda clumsy and it almost always hurts to
expose synchronize_rcu() in userland visible paths.
Wouldn't something like the following work?
* Remove bdi->usage_cnt and the synchronous waiting in
cgwb_bdi_destroy().
* Instead, make cgwb's hold bdi->refcnt and put it from
cgwb_release_workfn().
Then, we don't have to switch during shutdown and can just let things
drain.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 09/10] kobject: Export kobject_get_unless_zero()
From: Tejun Heo @ 2017-02-12 4:41 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-10-jack@suse.cz>
On Thu, Feb 09, 2017 at 01:44:32PM +0100, Jan Kara wrote:
> Make the function available for outside use and fortify it against NULL
> kobject.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Cc Greg?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 10/10] block: Fix oops scsi_disk_get()
From: Tejun Heo @ 2017-02-12 4:43 UTC (permalink / raw)
To: Jan Kara
Cc: Jens Axboe, linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170209124433.2626-11-jack@suse.cz>
On Thu, Feb 09, 2017 at 01:44:33PM +0100, Jan Kara wrote:
> When device open races with device shutdown, we can get the following
> oops in scsi_disk_get():
>
> [11863.044351] general protection fault: 0000 [#1] SMP
> [11863.045561] Modules linked in: scsi_debug xfs libcrc32c netconsole btrfs raid6_pq zlib_deflate lzo_compress xor [last unloaded: loop]
> [11863.047853] CPU: 3 PID: 13042 Comm: hald-probe-stor Tainted: G W 4.10.0-rc2-xen+ #35
> [11863.048030] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [11863.048030] task: ffff88007f438200 task.stack: ffffc90000fd0000
> [11863.048030] RIP: 0010:scsi_disk_get+0x43/0x70
> [11863.048030] RSP: 0018:ffffc90000fd3a08 EFLAGS: 00010202
> [11863.048030] RAX: 6b6b6b6b6b6b6b6b RBX: ffff88007f56d000 RCX: 0000000000000000
> [11863.048030] RDX: 0000000000000001 RSI: 0000000000000004 RDI: ffffffff81a8d880
> [11863.048030] RBP: ffffc90000fd3a18 R08: 0000000000000000 R09: 0000000000000001
> [11863.059217] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000fffffffa
> [11863.059217] R13: ffff880078872800 R14: ffff880070915540 R15: 000000000000001d
> [11863.059217] FS: 00007f2611f71800(0000) GS:ffff88007f0c0000(0000) knlGS:0000000000000000
> [11863.059217] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [11863.059217] CR2: 000000000060e048 CR3: 00000000778d4000 CR4: 00000000000006e0
> [11863.059217] Call Trace:
> [11863.059217] ? disk_get_part+0x22/0x1f0
> [11863.059217] sd_open+0x39/0x130
> [11863.059217] __blkdev_get+0x69/0x430
> [11863.059217] ? bd_acquire+0x7f/0xc0
> [11863.059217] ? bd_acquire+0x96/0xc0
> [11863.059217] ? blkdev_get+0x350/0x350
> [11863.059217] blkdev_get+0x126/0x350
> [11863.059217] ? _raw_spin_unlock+0x2b/0x40
> [11863.059217] ? bd_acquire+0x7f/0xc0
> [11863.059217] ? blkdev_get+0x350/0x350
> [11863.059217] blkdev_open+0x65/0x80
> ...
>
> As you can see RAX value is already poisoned showing that gendisk we got
> is already freed. The problem is that get_gendisk() looks up device
> number in ext_devt_idr and then does get_disk() which does kobject_get()
> on the disks kobject. However the disk gets removed from ext_devt_idr
> only in disk_release() (through blk_free_devt()) at which moment it has
> already 0 refcount and is already on its way to be freed. Indeed we've
> got a warning from kobject_get() about 0 refcount shortly before the
> oops.
>
> We fix the problem by using kobject_get_unless_zero() in get_disk() so
> that get_disk() cannot get reference on a disk that is already being
> freed.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 00/16] multiqueue for MMC/SD third try
From: Linus Walleij @ 2017-02-12 16:16 UTC (permalink / raw)
To: Avri Altman
Cc: linux-mmc@vger.kernel.org, Ulf Hansson, Adrian Hunter,
Paolo Valente, Chunyan Zhang, Baolin Wang,
linux-block@vger.kernel.org, Jens Axboe, Christoph Hellwig,
Arnd Bergmann
In-Reply-To: <9BEFCB713C10BF4B91D714797ECA5EEA117E08F5@ULS-OP-MBXIP04.sdcorp.global.sandisk.com>
On Sat, Feb 11, 2017 at 2:03 PM, Avri Altman <Avri.Altman@sandisk.com> wrote:
>>
>> The iozone results seem a bit consistent and all values seem to be noisy and
>> not say much. I don't know why really, maybe the test is simply not relevant,
>> the tests don't seem to be significantly affected by any of the patches, so
>> let's focus on the dd and find tests.
>
> Maybe use a more selective testing mode instead of -az.
> Also maybe you want to clear the cache between the sequential and random tests:
> #sync
> #echo 3 > /proc/sys/vm/drop_caches
> #sync
> It helps to obtain a more robust results.
OK I'll try that. I actually cold booted the system between each test to
avoid cache effects.
>> What immediately jumps out at you is that linear read/writes perform just as
>> nicely or actually better with MQ than with the old block layer.
>
> How come 22.7MB/s before vs. 22.1MB/s after is better? or did I misunderstand the output?
> Also as dd is probably using the buffer cache, unlike the iozone test in which you properly used -I
> for direct mode to isolate the blk-mq effect - does it really say much?
Sorry I guess I was a bit too enthusiastic there. The difference is in
the error margin, it is just based on a single test. I guess I should re-run
them with a few iterations, then drop caches iterate drop caches iterate
and get some more stable figures.
We need to understand what is meant by "better" too:
quicker compared to wall clock time (real), user
or sys.
So for the dd command:
real user sys
Before patches: 45.13 0.02 7.60
Move asynch pp 52.17 0.01 6.96
Issue in parallel 49.31 0.00 7.11
Multiqueue 46.25 0.03 6.42
For these pure kernel patches only the last figure (sys) is really relevant
IIUC. The other figures are just system noise, but still the eventual
throughput figure from dd is including the time spent on other processes
in the system etc, so that value is not relevant.
But I guess Paolo may need to beat me up a bit here: what the user
percieves in the end if of course the most relevant for any human ...
Nevertheless if we just look at sys then MQ is already winning this test.
I just think there is too little tested here.
I think 1GiB is maybe too little. Maybe I need to read the entire card
a few times or something?
Since dd is just using sequenctially blocks from mmcblk0 on a cold
booted system I think the buffer cache is empty except for maybe
the partition table blocks. But I dunno. I will use your trick the next
time to drop caches.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH V4 1/2] uapi: sed-opal fix IOW for activate lsp to use correct struct
From: Christoph Hellwig @ 2017-02-13 7:50 UTC (permalink / raw)
To: Scott Bauer
Cc: keith.busch, arnd, hch, linux-kernel, linux-nvme, axboe,
David.Laight, linux-block, jonathan.derrick
In-Reply-To: <1486742454-3254-2-git-send-email-scott.bauer@intel.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH V4 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Christoph Hellwig @ 2017-02-13 7:54 UTC (permalink / raw)
To: Scott Bauer
Cc: linux-nvme, David.Laight, arnd, axboe, keith.busch,
jonathan.derrick, hch, linux-kernel, linux-block
In-Reply-To: <1486742454-3254-3-git-send-email-scott.bauer@intel.com>
> int sed_ioctl(struct opal_dev *dev, unsigned int cmd, unsigned long ptr)
> {
> + void *ioctl_ptr;
> + int ret = -ENOTTY;
> void __user *arg = (void __user *)ptr;
Can we use this opportunity to clean up the usual ioctl argument mess.
Id say pass the "void __user *argp" argument already (nvme_ioctl should
have a local variable for it anyway), and then just rename ioctl_ptr
variable to the usual short p.
> + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> + ioctl_ptr = memdup_user(arg, cmd_size);
cmd_size is only used once, so why not opencode the expression in the
argument to memdup_user.
^ permalink raw reply
* Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order radix-tree entries
From: Kirill A. Shutemov @ 2017-02-13 13:43 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kirill A. Shutemov, Theodore Ts'o, Andreas Dilger, Jan Kara,
Andrew Morton, Alexander Viro, Hugh Dickins, Andrea Arcangeli,
Dave Hansen, Vlastimil Babka, Ross Zwisler, linux-ext4,
linux-fsdevel, linux-kernel, linux-mm, linux-block
In-Reply-To: <20170209165820.GA12768@node.shutemov.name>
On Thu, Feb 09, 2017 at 07:58:20PM +0300, Kirill A. Shutemov wrote:
> I'll look into it.
I ended up with this (I'll test it more later):
void filemap_map_pages(struct vm_fault *vmf,
pgoff_t start_pgoff, pgoff_t end_pgoff)
{
struct radix_tree_iter iter;
void **slot;
struct file *file = vmf->vma->vm_file;
struct address_space *mapping = file->f_mapping;
pgoff_t last_pgoff = start_pgoff;
loff_t size;
struct page *page;
bool mapped;
rcu_read_lock();
radix_tree_for_each_slot(slot, &mapping->page_tree, &iter,
start_pgoff) {
unsigned long index = iter.index;
if (index < start_pgoff)
index = start_pgoff;
if (index > end_pgoff)
break;
repeat:
page = radix_tree_deref_slot(slot);
if (unlikely(!page))
continue;
if (radix_tree_exception(page)) {
if (radix_tree_deref_retry(page))
slot = radix_tree_iter_retry(&iter);
continue;
}
if (!page_cache_get_speculative(page))
goto repeat;
/* Has the page moved? */
if (unlikely(page != *slot)) {
put_page(page);
goto repeat;
}
/* For multi-order entries, find relevant subpage */
page = find_subpage(page, index);
if (!PageUptodate(page) || PageReadahead(page))
goto skip;
if (!trylock_page(page))
goto skip;
if (page_mapping(page) != mapping || !PageUptodate(page))
goto skip_unlock;
size = round_up(i_size_read(mapping->host), PAGE_SIZE);
if (compound_head(page)->index >= size >> PAGE_SHIFT)
goto skip_unlock;
if (file->f_ra.mmap_miss > 0)
file->f_ra.mmap_miss--;
map_next_subpage:
if (PageHWPoison(page))
goto next;
vmf->address += (index - last_pgoff) << PAGE_SHIFT;
if (vmf->pte)
vmf->pte += index - last_pgoff;
last_pgoff = index;
mapped = !alloc_set_pte(vmf, NULL, page);
/* Huge page is mapped or last index? No need to proceed. */
if (pmd_trans_huge(*vmf->pmd) ||
index == end_pgoff) {
unlock_page(page);
break;
}
next:
if (page && PageCompound(page)) {
/* Last subpage handled? */
if ((index & (compound_nr_pages(page) - 1)) ==
compound_nr_pages(page) - 1)
goto skip_unlock;
index++;
page++;
/*
* One page reference goes to page table mapping.
* Need additional reference, if last alloc_set_pte()
* succeed.
*/
if (mapped)
get_page(page);
goto map_next_subpage;
}
skip_unlock:
unlock_page(page);
skip:
iter.index = compound_head(page)->index +
compound_nr_pages(page) - 1;
/* Only give up reference if alloc_set_pte() failed. */
if (!mapped)
put_page(page);
}
rcu_read_unlock();
}
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v1 3/5] md: fail if mddev->bio_set can't be created
From: Christoph Hellwig @ 2017-02-13 13:45 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1486724177-14817-4-git-send-email-tom.leiming@gmail.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
but this really needs to be patch 2 in this series.
^ permalink raw reply
* Re: [PATCH v1 1/5] block: introduce bio_clone_bioset_partial()
From: Christoph Hellwig @ 2017-02-13 13:46 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1486724177-14817-2-git-send-email-tom.leiming@gmail.com>
On Fri, Feb 10, 2017 at 06:56:13PM +0800, Ming Lei wrote:
> md still need bio clone(not the fast version) for behind write,
> and it is more efficient to use bio_clone_bioset_partial().
>
> The idea is simple and just copy the bvecs range specified from
> parameters.
Given how few users bio_clone_bioset has I wonder if we shouldn't
simply add the two new arguments to it instead of adding another
indirection.
Otherwise looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v1 2/5] md/raid1: use bio_clone_bioset_partial() in case of write behind
From: Christoph Hellwig @ 2017-02-13 13:48 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1486724177-14817-3-git-send-email-tom.leiming@gmail.com>
> + struct bio *mbio = NULL;
> + int offset;
> if (!r1_bio->bios[i])
> continue;
>
> - mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
> - bio_trim(mbio, r1_bio->sector - bio->bi_iter.bi_sector,
> - max_sectors);
> + offset = r1_bio->sector - bio->bi_iter.bi_sector;
I think offset should be a sector_t.
Otherwise this looks fine:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v1 4/5] md: remove unnecessary check on mddev
From: Christoph Hellwig @ 2017-02-13 13:49 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1486724177-14817-5-git-send-email-tom.leiming@gmail.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply
* Re: [PATCH v1 5/5] md: fast clone bio in bio_clone_mddev()
From: Christoph Hellwig @ 2017-02-13 13:49 UTC (permalink / raw)
To: Ming Lei
Cc: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
In-Reply-To: <1486724177-14817-6-git-send-email-tom.leiming@gmail.com>
Please use bio_clone_fast directly and kill the wrapper.
^ permalink raw reply
* Re: [PATCHv6 03/37] page-flags: relax page flag policy for few flags
From: Kirill A. Shutemov @ 2017-02-13 13:59 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kirill A. Shutemov, Theodore Ts'o, Andreas Dilger, Jan Kara,
Andrew Morton, Alexander Viro, Hugh Dickins, Andrea Arcangeli,
Dave Hansen, Vlastimil Babka, Ross Zwisler, linux-ext4,
linux-fsdevel, linux-kernel, linux-mm, linux-block
In-Reply-To: <20170209040113.GR2267@bombadil.infradead.org>
On Wed, Feb 08, 2017 at 08:01:13PM -0800, Matthew Wilcox wrote:
> On Thu, Jan 26, 2017 at 02:57:45PM +0300, Kirill A. Shutemov wrote:
> > These flags are in use for filesystems with backing storage: PG_error,
> > PG_writeback and PG_readahead.
>
> Oh ;-) Then I amend my comment on patch 1 to be "patch 3 needs to go
> ahead of patch 1" ;-)
It doesn't really matter as long as both before patch 37 :P
--
Kirill A. Shutemov
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox