* Re: [lkp-robot] [scsi, block] 0dba1314d4: WARNING:at_fs/sysfs/dir.c:#sysfs_warn_dup
From: James Bottomley @ 2017-02-09 0:08 UTC (permalink / raw)
To: Dan Williams, Jens Axboe
Cc: Christoph Hellwig, 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: <CAPcyv4jkak-gX3gQaBJiEk1G79skdN2F9WL-sK2DjPwFhA8rdg@mail.gmail.com>
On Mon, 2017-02-06 at 21:42 -0800, Dan Williams wrote:
> On Mon, Feb 6, 2017 at 8:09 PM, Jens Axboe <axboe@fb.com> 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.
> >
> > This is running linux-next? What's your .config?
> >
>
> The original failure report is here:
>
> http://marc.info/?l=linux-kernel&m=148619222300774&w=2
>
> ...but it reproduces on current mainline with the same config. I
> haven't spotted what makes scsi_debug behave like this.
Looking at the config, it's a static debug with report luns enabled.
Is it as simple as the fact that we probe lun 0 manually to see if the
target exists, but then we don't account for the fact that we already
did this, so if it turns up again in the report lun scan, we'll probe
it again leading to a double add. If that theory is correct, this may
be the fix (compile tested only).
James
---
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6f7128f..ba4be08 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1441,6 +1441,10 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
for (lunp = &lun_data[1]; lunp <= &lun_data[num_luns]; lunp++) {
lun = scsilun_to_int(lunp);
+ if (lun == 0)
+ /* already scanned LUN 0 */
+ continue;
+
if (lun > sdev->host->max_lun) {
sdev_printk(KERN_WARNING, sdev,
"lun%llu has a LUN larger than"
^ permalink raw reply related
* Re: [PATCHv6 01/37] mm, shmem: swich huge tmpfs to multi-order radix-tree entries
From: Matthew Wilcox @ 2017-02-09 3:57 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-2-kirill.shutemov@linux.intel.com>
On Thu, Jan 26, 2017 at 02:57:43PM +0300, Kirill A. Shutemov wrote:
> +++ b/include/linux/pagemap.h
> @@ -332,6 +332,15 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> mapping_gfp_mask(mapping));
> }
>
> +static inline struct page *find_subpage(struct page *page, pgoff_t offset)
> +{
> + VM_BUG_ON_PAGE(PageTail(page), page);
> + VM_BUG_ON_PAGE(page->index > offset, page);
> + VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) < offset,
> + page);
> + return page - page->index + offset;
> +}
What would you think to:
static inline void check_page_index(struct page *page, pgoff_t offset)
{
VM_BUG_ON_PAGE(PageTail(page), page);
VM_BUG_ON_PAGE(page->index > offset, page);
VM_BUG_ON_PAGE(page->index + (1 << compound_order(page)) <= offset,
page);
}
(I think I fixed an off-by-one error up there ... if
index + (1 << order) == offset, this is also a bug, right?
because offset would then refer to the next page, not this page)
static inline struct page *find_subpage(struct page *page, pgoff_t offset)
{
check_page_index(page, offset);
return page + (offset - page->index);
}
... then you can use check_page_index down ...
> @@ -1250,7 +1233,6 @@ struct page *find_lock_entry(struct address_space *mapping, pgoff_t offset)
> put_page(page);
> goto repeat;
> }
> - VM_BUG_ON_PAGE(page_to_pgoff(page) != offset, page);
... here?
> @@ -1472,25 +1451,35 @@ unsigned find_get_pages(struct address_space *mapping, pgoff_t start,
> goto repeat;
> }
>
> + /* For multi-order entries, find relevant subpage */
> + if (PageTransHuge(page)) {
> + VM_BUG_ON(index - page->index < 0);
> + VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> + page += index - page->index;
> + }
Use find_subpage() here?
> pages[ret] = page;
> if (++ret == nr_pages)
> break;
> + if (!PageTransCompound(page))
> + continue;
> + for (refs = 0; ret < nr_pages &&
> + (index + 1) % HPAGE_PMD_NR;
> + ret++, refs++, index++)
> + pages[ret] = ++page;
> + if (refs)
> + page_ref_add(compound_head(page), refs);
> + if (ret == nr_pages)
> + break;
Can we avoid referencing huge pages specifically in the page cache? I'd
like us to get to the point where we can put arbitrary compound pages into
the page cache. For example, I think this can be written as:
if (!PageCompound(page))
continue;
for (refs = 0; ret < nr_pages; refs++, index++) {
if (index > page->index + (1 << compound_order(page)))
break;
pages[ret++] = ++page;
}
if (refs)
page_ref_add(compound_head(page), refs);
if (ret == nr_pages)
break;
> @@ -1541,19 +1533,12 @@ unsigned find_get_pages_contig(struct address_space *mapping, pgoff_t index,
>
> + /* For multi-order entries, find relevant subpage */
> + if (PageTransHuge(page)) {
> + VM_BUG_ON(index - page->index < 0);
> + VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> + page += index - page->index;
> + }
> +
> pages[ret] = page;
> if (++ret == nr_pages)
> break;
> + if (!PageTransCompound(page))
> + continue;
> + for (refs = 0; ret < nr_pages &&
> + (index + 1) % HPAGE_PMD_NR;
> + ret++, refs++, index++)
> + pages[ret] = ++page;
> + if (refs)
> + page_ref_add(compound_head(page), refs);
> + if (ret == nr_pages)
> + break;
> }
> rcu_read_unlock();
> return ret;
Ugh, the same code again. Hmm ... we only need to modify 'ret' as a result
of this ... so could we split it out like this?
static unsigned populate_pages(struct page **pages, unsigned i, unsigned max,
struct page *page)
{
unsigned refs = 0;
for (;;) {
pages[i++] = page;
if (i == max)
break;
if (PageHead(page + 1))
break;
page++;
refs++;
}
if (refs)
page_ref_add(compound_head(page), refs);
return i;
}
> +unsigned find_get_pages_tag(struct address_space *mapping, pgoff_t *indexp,
> int tag, unsigned int nr_pages, struct page **pages)
> break;
> + if (!PageTransCompound(page))
> + continue;
> + for (refs = 0; ret < nr_pages &&
> + (index + 1) % HPAGE_PMD_NR;
> + ret++, refs++, index++)
> + pages[ret] = ++page;
> + if (refs)
> + page_ref_add(compound_head(page), refs);
> + if (ret == nr_pages)
> + break;
> }
... and again!
> @@ -2326,25 +2337,26 @@ void filemap_map_pages(struct vm_fault *vmf,
> + /* For multi-order entries, find relevant subpage */
> + if (PageTransHuge(page)) {
> + VM_BUG_ON(index - page->index < 0);
> + VM_BUG_ON(index - page->index >= HPAGE_PMD_NR);
> + page += index - page->index;
> + }
> +
> if (!PageUptodate(page) ||
> PageReadahead(page) ||
Readahead is PF_NO_COMPOUND ... so I don't see why this works?
> @@ -2378,8 +2390,14 @@ void filemap_map_pages(struct vm_fault *vmf,
> /* Huge page is mapped? No need to proceed. */
> if (pmd_trans_huge(*vmf->pmd))
> break;
> - if (iter.index == end_pgoff)
> + if (index == end_pgoff)
> break;
> + if (page && PageTransCompound(page) &&
> + (index & (HPAGE_PMD_NR - 1)) !=
> + HPAGE_PMD_NR - 1) {
> + index++;
> + goto repeat;
Do we really have to go all the way back to the beginning of the loop? It'd
be nice to be able to insert all of the relevant PTEs in a shorter loop here.
We'd need to bump the reference count N more times, and I think we do need
to check HWPoison for each subpage.
^ permalink raw reply
* Re: [PATCHv6 03/37] page-flags: relax page flag policy for few flags
From: Matthew Wilcox @ 2017-02-09 4:01 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-4-kirill.shutemov@linux.intel.com>
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" ;-)
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
> include/linux/page-flags.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 6b5818d6de32..b85b73cfb1b3 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -263,7 +263,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; }
>
> __PAGEFLAG(Locked, locked, PF_NO_TAIL)
> PAGEFLAG(Waiters, waiters, PF_ONLY_HEAD) __CLEARPAGEFLAG(Waiters, waiters, PF_ONLY_HEAD)
> -PAGEFLAG(Error, error, PF_NO_COMPOUND) TESTCLEARFLAG(Error, error, PF_NO_COMPOUND)
> +PAGEFLAG(Error, error, PF_NO_TAIL) TESTCLEARFLAG(Error, error, PF_NO_TAIL)
> PAGEFLAG(Referenced, referenced, PF_HEAD)
> TESTCLEARFLAG(Referenced, referenced, PF_HEAD)
> __SETPAGEFLAG(Referenced, referenced, PF_HEAD)
> @@ -303,15 +303,15 @@ PAGEFLAG(OwnerPriv1, owner_priv_1, PF_ANY)
> * Only test-and-set exist for PG_writeback. The unconditional operators are
> * risky: they bypass page accounting.
> */
> -TESTPAGEFLAG(Writeback, writeback, PF_NO_COMPOUND)
> - TESTSCFLAG(Writeback, writeback, PF_NO_COMPOUND)
> +TESTPAGEFLAG(Writeback, writeback, PF_NO_TAIL)
> + TESTSCFLAG(Writeback, writeback, PF_NO_TAIL)
> PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
>
> /* PG_readahead is only used for reads; PG_reclaim is only for writes */
> PAGEFLAG(Reclaim, reclaim, PF_NO_TAIL)
> TESTCLEARFLAG(Reclaim, reclaim, PF_NO_TAIL)
> -PAGEFLAG(Readahead, reclaim, PF_NO_COMPOUND)
> - TESTCLEARFLAG(Readahead, reclaim, PF_NO_COMPOUND)
> +PAGEFLAG(Readahead, reclaim, PF_NO_TAIL)
> + TESTCLEARFLAG(Readahead, reclaim, PF_NO_TAIL)
>
> #ifdef CONFIG_HIGHMEM
> /*
> --
> 2.11.0
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Christoph Hellwig @ 2017-02-09 7:18 UTC (permalink / raw)
To: Scott Bauer
Cc: keith.busch, hch, linux-kernel, linux-nvme, axboe, linux-block,
jonathan.derrick
In-Reply-To: <1486594815-2427-1-git-send-email-scott.bauer@intel.com>
I'm really not a fan of adding a global synchronization point for
no good reason. Arnd's original patch looks fine to me, but if you
really don't want it I'd suggest to use memdup_user and just
dynamically allocate the structure.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* RE: Boot regression (was "Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements")
From: Dexuan Cui @ 2017-02-09 7:35 UTC (permalink / raw)
To: hch@lst.de, Jens Axboe
Cc: Bart Van Assche, hare@suse.com, hare@suse.de, Martin K. Petersen,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
jth@kernel.org
In-Reply-To: <20170208180314.GA17838@lst.de>
> From: hch@lst.de [mailto:hch@lst.de]
> Sent: Thursday, February 9, 2017 02:03
> To: Jens Axboe <axboe@kernel.dk>
> Cc: Dexuan Cui <decui@microsoft.com>; Bart Van Assche
> <Bart.VanAssche@sandisk.com>; hare@suse.com; hare@suse.de; Martin K.
> Petersen <martin.petersen@oracle.com>; hch@lst.de; linux-
> kernel@vger.kernel.org; linux-block@vger.kernel.org; jth@kernel.org
> Subject: Re: Boot regression (was "Re: [PATCH] genhd: Do not hold event l=
ock
> when scheduling workqueue elements")
>=20
> On Wed, Feb 08, 2017 at 10:43:59AM -0700, Jens Axboe wrote:
> > I've changed the subject line, this issue has nothing to do with the
> > issue that Hannes was attempting to fix.
>=20
> Nothing really useful in the thread. Dexuan, can you throw in some
> prints to see which command times out?
My colleagues have sent you the logs in another thread.
Thanks for looking into this!
-- Dexuan
^ permalink raw reply
* rate-capped fio jobs in a CFQ group degrade performance of fio jobs in another CFQ group with the same weight
From: Hou Tao @ 2017-02-09 9:40 UTC (permalink / raw)
To: linux-block; +Cc: axboe, Shaohua Li, tj, Vivek Goyal
Hi all,
During our test of the CFQ group schedule, we found a performance related problem.
Rate-capped fio jobs in a CFQ group will degrade the performance of fio jobs in
another CFQ group. Both of the CFQ groups have the same blkio.weight.
We launch two fios in difference terminals. The following is the content of
these two job files:
# cat a.job
[global]
bs=4k
ioengine=psync
iodepth=1
direct=1
rw=randwrite
time_based
runtime=30
cgroup_nodelete=1
group_reporting=1
[test1]
filename=test1.dat
size=2G
cgroup_weight=500
cgroup=testA
thread=1
numjobs=2
[test3]
stonewall
filename=test3.dat
size=2G
rate=4m
cgroup_weight=500
cgroup=testA
thread=1
numjobs=2
# cat b.job
[global]
bs=4k
ioengine=psync
iodepth=1
direct=1
rw=randwrite
time_based
runtime=60
cgroup_nodelete=1
group_reporting=1
[test2]
filename=test2.dat
size=2G
cgroup_weight=500
cgroup=testB
thread=1
numjobs=2
In the first 30 seconds, the iops of both "test1" and "test3" is ~5000 and
the rate is about 20MBps.
In the last 30 seconds, we start "test3" and the rate of "test3" is limited to
8MBps (4MBps * 2 jobs). The cap works fine, but the iops of "test2" also
degraded from ~5000 to ~2040 or lower. This may seems reasonable for CFQ group
schedule because it needs to dispatch disk time equally to these two CFQ group,
but it doesn't fully utilize the disk throughput.
It can be reproduced on 4.4 and 4.10.0-rc7, the version of fio is 2.2.4.
The configuration options of CFQ are untouched.
I am not sure whether this situation is a bug or not. Are there some
configuration options we can used to alleviate the performance degradation
problem ? And how CFQ group schedule handles the trace-off between fairness
and performance ?
Any suggestion will be appreciated.
Regards,
Tao
^ permalink raw reply
* Re: [PATCH 1/1] dm-zoned: Zoned block device target
From: Christoph Hellwig @ 2017-02-09 9:36 UTC (permalink / raw)
To: Damien Le Moal
Cc: dm-devel, Alasdair Kergon, Mike Snitzer, Hannes Reinecke,
Christoph Hellwig, linux-block
In-Reply-To: <20170209041849.30964-1-damien.lemoal@wdc.com>
On Thu, Feb 09, 2017 at 01:18:49PM +0900, Damien Le Moal wrote:
> +
> +/*
> + * Target BIO completion.
> + */
> +static inline void dmz_bio_end(struct bio *bio, int err)
> +{
> + struct dm_zone_bioctx *bioctx =
> + dm_per_bio_data(bio, sizeof(struct dm_zone_bioctx));
> +
> + if (atomic_dec_and_test(&bioctx->ref)) {
It seems like this could be simplified a bit by using bio_chain
to chain the clones to the original bio.
^ permalink raw reply
* Re: [PATCH 22/24] ubifs: Convert to separately allocated bdi
From: Jan Kara @ 2017-02-09 12:17 UTC (permalink / raw)
To: Richard Weinberger
Cc: Jan Kara, linux-fsdevel, Christoph Hellwig, linux-block,
Artem Bityutskiy, Adrian Hunter, linux-mtd
In-Reply-To: <d0d1d396-a340-866b-c931-974fa8ea53a1@nod.at>
On Wed 08-02-17 12:24:00, Richard Weinberger wrote:
> Am 02.02.2017 um 18:34 schrieb Jan Kara:
> > Allocate struct backing_dev_info separately instead of embedding it
> > inside the superblock. This unifies handling of bdi among users.
> >
> > CC: Richard Weinberger <richard@nod.at>
> > CC: Artem Bityutskiy <dedekind1@gmail.com>
> > CC: Adrian Hunter <adrian.hunter@intel.com>
> > CC: linux-mtd@lists.infradead.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/ubifs/super.c | 23 +++++++----------------
> > fs/ubifs/ubifs.h | 3 ---
> > 2 files changed, 7 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> > index e08aa04fc835..34810eb52b22 100644
> > --- a/fs/ubifs/super.c
> > +++ b/fs/ubifs/super.c
> > @@ -1827,7 +1827,6 @@ static void ubifs_put_super(struct super_block *sb)
> > }
> >
> > ubifs_umount(c);
> > - bdi_destroy(&c->bdi);
> > ubi_close_volume(c->ubi);
> > mutex_unlock(&c->umount_mutex);
> > }
> > @@ -2019,29 +2018,23 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
> > goto out;
> > }
> >
> > + err = ubifs_parse_options(c, data, 0);
> > + if (err)
> > + goto out_close;
> > +
> > /*
> > * UBIFS provides 'backing_dev_info' in order to disable read-ahead. For
> > * UBIFS, I/O is not deferred, it is done immediately in readpage,
> > * which means the user would have to wait not just for their own I/O
> > * but the read-ahead I/O as well i.e. completely pointless.
> > *
> > - * Read-ahead will be disabled because @c->bdi.ra_pages is 0.
> > + * Read-ahead will be disabled because @sb->s_bdi->ra_pages is 0.
> > */
> > - c->bdi.name = "ubifs",
> > - c->bdi.capabilities = 0;
>
> So ->capabilities is now zero by default since you use __GFP_ZERO in
> bdi_alloc().
> At least for UBIFS I'll add a comment on this, otherwise it is not so
> clear that UBIFS wants a BDI with no capabilities and how it achieves that.
OK, I've modified the comment to:
* Read-ahead will be disabled because @sb->s_bdi->ra_pages is 0. Also
* @sb->s_bdi->capabilities are initialized to 0 so there won't be any
* writeback happening.
*/
> Acked-by: Richard Weinberger <richard@nod.at>
Thanks.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [lustre-devel] [PATCH 04/24] fs: Provide infrastructure for dynamic BDIs in filesystems
From: Jan Kara @ 2017-02-09 12:12 UTC (permalink / raw)
To: Dilger, Andreas
Cc: Jan Kara, linux-fsdevel@vger.kernel.org,
linux-block@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-nilfs@vger.kernel.org, linux-cifs@vger.kernel.org,
ecryptfs@vger.kernel.org, codalist@coda.cs.cmu.edu,
Christoph Hellwig, cluster-devel@redhat.com,
linux-mtd@lists.infradead.org, osd-dev@open-osd.org,
v9fs-developer@lists.sourceforge.net, ceph-devel@vger.kernel.org,
Petr Vandrovec, linux-afs@lists.infradead.org,
linux-btrfs@vger.kernel.org, lustre-devel@lists.lustre.org
In-Reply-To: <C01BF300-AD67-4A3E-810F-AAB2EEB90B97@intel.com>
> > @@ -1249,6 +1254,50 @@ mount_fs(struct file_system_type *type, int flags, const char *name, void *data)
> > }
> >
> > /*
> > + * Setup private BDI for given superblock. I gets automatically cleaned up
>
> (typo) s/I/It/
>
> Looks fine otherwise.
Thanks, fixed.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH 0/10] block: Fix block device shutdown related races
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
Hello,
this patch set fixes several different races and issues I've found when testing
device shutdown and reuse. The first three patches and fixes to problems in my
previous series fixing BDI lifetime issues. Patch 4 fixes issues with reuse of
BDI name with scsi devices. With it I cannot reproduce the BDI name reuse
issues using Omar's stress test using scsi_debug so it can be used as a
replacement of Dan's patches. Patches 5-8 fix oops that is triggered by
__blkdev_put() calling inode_detach_wb() too early (the problem reported by
Thiago). Patches 9 and 10 fix oops due to a bug in gendisk code where
get_gendisk() can return already freed gendisk structure (again triggered by
Omar's stress test).
People, please have a look at patches. The are mostly simple however the
interactions are rather complex so I may have missed something. Also I'm
happy for any additional testing these patches can get - I've stressed them
with Omar's script, tested memcg writeback, tested static (not udev managed)
device inodes.
Honza
^ permalink raw reply
* [PATCH 01/10] block: Move bdev_unhash_inode() after invalidate_partition()
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
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>
---
block/genhd.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index d9ccd42f3675..6cb9f3a34a92 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -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);
}
disk_part_iter_exit(&piter);
--
2.10.2
^ permalink raw reply related
* [PATCH 02/10] block: Unhash also block device inode for the whole device
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
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>
---
block/genhd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/genhd.c b/block/genhd.c
index 6cb9f3a34a92..f6c4d4400759 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -655,6 +655,7 @@ void del_gendisk(struct gendisk *disk)
disk_part_iter_exit(&piter);
invalidate_partition(disk, 0);
+ bdev_unhash_inode(disk_devt(disk));
set_capacity(disk, 0);
disk->flags &= ~GENHD_FL_UP;
--
2.10.2
^ permalink raw reply related
* [PATCH 03/10] block: Revalidate i_bdev reference in bd_aquire()
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
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>
---
fs/block_dev.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 601b71b76d7f..360439373a66 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1043,13 +1043,22 @@ static struct block_device *bd_acquire(struct inode *inode)
spin_lock(&bdev_lock);
bdev = inode->i_bdev;
- if (bdev) {
+ if (bdev && !inode_unhashed(bdev->bd_inode)) {
bdgrab(bdev);
spin_unlock(&bdev_lock);
return bdev;
}
spin_unlock(&bdev_lock);
+ /*
+ * i_bdev references block device inode that was already shut down
+ * (corresponding device got removed). Remove the reference and look
+ * up block device inode again just in case new device got
+ * reestablished under the same device number.
+ */
+ if (bdev)
+ bd_forget(bdev);
+
bdev = bdget(inode->i_rdev);
if (bdev) {
spin_lock(&bdev_lock);
--
2.10.2
^ permalink raw reply related
* [PATCH 04/10] block: Move bdi_unregister() to del_gendisk()
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
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>
---
block/blk-core.c | 2 --
block/genhd.c | 7 +++++++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 47104f6a398b..9a901dcfdd5c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -580,8 +580,6 @@ void blk_cleanup_queue(struct request_queue *q)
q->queue_lock = &q->__queue_lock;
spin_unlock_irq(lock);
- bdi_unregister(q->backing_dev_info);
-
/* @q is and will stay empty, shutdown and put */
blk_put_queue(q);
}
diff --git a/block/genhd.c b/block/genhd.c
index f6c4d4400759..68c613edb93a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -660,6 +660,13 @@ void del_gendisk(struct gendisk *disk)
disk->flags &= ~GENHD_FL_UP;
sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
+ /*
+ * Unregister bdi before releasing device numbers (as they can get
+ * reused and we'd get clashes in sysfs) but after bdev inodes are
+ * unhashed and thus will be soon destroyed as bdev inode's reference
+ * to wb_writeback can block bdi_unregister().
+ */
+ bdi_unregister(disk->queue->backing_dev_info);
blk_unregister_queue(disk);
blk_unregister_region(disk_devt(disk), disk->minors);
--
2.10.2
^ permalink raw reply related
* [PATCH 05/10] writeback: Generalize and standardize I_SYNC waiting function
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
__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>
---
fs/fs-writeback.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index ef600591d96f..c9770de11650 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1170,21 +1170,16 @@ static int write_inode(struct inode *inode, struct writeback_control *wbc)
}
/*
- * Wait for writeback on an inode to complete. Called with i_lock held.
+ * Wait for bit in inode->i_state to clear. Called with i_lock held.
* Caller must make sure inode cannot go away when we drop i_lock.
*/
-static void __inode_wait_for_writeback(struct inode *inode)
+static void __inode_wait_for_state_bit(struct inode *inode, int bit_nr)
__releases(inode->i_lock)
__acquires(inode->i_lock)
{
- DEFINE_WAIT_BIT(wq, &inode->i_state, __I_SYNC);
- wait_queue_head_t *wqh;
-
- wqh = bit_waitqueue(&inode->i_state, __I_SYNC);
- while (inode->i_state & I_SYNC) {
+ while (inode->i_state & (1 << bit_nr)) {
spin_unlock(&inode->i_lock);
- __wait_on_bit(wqh, &wq, bit_wait,
- TASK_UNINTERRUPTIBLE);
+ wait_on_bit(&inode->i_state, bit_nr, TASK_UNINTERRUPTIBLE);
spin_lock(&inode->i_lock);
}
}
@@ -1195,7 +1190,7 @@ static void __inode_wait_for_writeback(struct inode *inode)
void inode_wait_for_writeback(struct inode *inode)
{
spin_lock(&inode->i_lock);
- __inode_wait_for_writeback(inode);
+ __inode_wait_for_state_bit(inode, __I_SYNC);
spin_unlock(&inode->i_lock);
}
@@ -1397,7 +1392,7 @@ static int writeback_single_inode(struct inode *inode,
* inode reference or inode has I_WILL_FREE set, it cannot go
* away under us.
*/
- __inode_wait_for_writeback(inode);
+ __inode_wait_for_state_bit(inode, __I_SYNC);
}
WARN_ON(inode->i_state & I_SYNC);
/*
--
2.10.2
^ permalink raw reply related
* [PATCH 06/10] writeback: Move __inode_wait_for_state_bit
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
Move it up in fs/fs-writeback.c so that we don't have to use forward
declarations. No code change.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/fs-writeback.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index c9770de11650..23dc97cf2a50 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -99,6 +99,21 @@ static inline struct inode *wb_inode(struct list_head *head)
EXPORT_TRACEPOINT_SYMBOL_GPL(wbc_writepage);
+/*
+ * Wait for bit in inode->i_state to clear. Called with i_lock held.
+ * Caller must make sure inode cannot go away when we drop i_lock.
+ */
+static void __inode_wait_for_state_bit(struct inode *inode, int bit_nr)
+ __releases(inode->i_lock)
+ __acquires(inode->i_lock)
+{
+ while (inode->i_state & (1 << bit_nr)) {
+ spin_unlock(&inode->i_lock);
+ wait_on_bit(&inode->i_state, bit_nr, TASK_UNINTERRUPTIBLE);
+ spin_lock(&inode->i_lock);
+ }
+}
+
static bool wb_io_lists_populated(struct bdi_writeback *wb)
{
if (wb_has_dirty_io(wb)) {
@@ -1170,21 +1185,6 @@ static int write_inode(struct inode *inode, struct writeback_control *wbc)
}
/*
- * Wait for bit in inode->i_state to clear. Called with i_lock held.
- * Caller must make sure inode cannot go away when we drop i_lock.
- */
-static void __inode_wait_for_state_bit(struct inode *inode, int bit_nr)
- __releases(inode->i_lock)
- __acquires(inode->i_lock)
-{
- while (inode->i_state & (1 << bit_nr)) {
- spin_unlock(&inode->i_lock);
- wait_on_bit(&inode->i_state, bit_nr, TASK_UNINTERRUPTIBLE);
- spin_lock(&inode->i_lock);
- }
-}
-
-/*
* Wait for writeback on an inode to complete. Caller must have inode pinned.
*/
void inode_wait_for_writeback(struct inode *inode)
--
2.10.2
^ permalink raw reply related
* [PATCH 07/10] writeback: Implement reliable switching to default writeback structure
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
Currently switching of inode between different writeback structures is
asynchronous and not guaranteed to succeed. Add a variant of switching
that is synchronous and reliable so that it can reliably move inode to
the default writeback structure (bdi->wb) when writeback on bdi is going
to be shutdown.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/fs-writeback.c | 60 ++++++++++++++++++++++++++++++++++++++++-------
include/linux/fs.h | 3 ++-
include/linux/writeback.h | 6 +++++
3 files changed, 60 insertions(+), 9 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 23dc97cf2a50..52992a1036b1 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -332,14 +332,11 @@ struct inode_switch_wbs_context {
struct work_struct work;
};
-static void inode_switch_wbs_work_fn(struct work_struct *work)
+static void do_inode_switch_wbs(struct inode *inode,
+ struct bdi_writeback *new_wb)
{
- struct inode_switch_wbs_context *isw =
- container_of(work, struct inode_switch_wbs_context, work);
- struct inode *inode = isw->inode;
struct address_space *mapping = inode->i_mapping;
struct bdi_writeback *old_wb = inode->i_wb;
- struct bdi_writeback *new_wb = isw->new_wb;
struct radix_tree_iter iter;
bool switched = false;
void **slot;
@@ -436,15 +433,29 @@ static void inode_switch_wbs_work_fn(struct work_struct *work)
spin_unlock(&new_wb->list_lock);
spin_unlock(&old_wb->list_lock);
+ /*
+ * Make sure waitqueue_active() check in wake_up_bit() cannot happen
+ * before I_WB_SWITCH is cleared. Pairs with the barrier in
+ * set_task_state() after wait_on_bit() added waiter to the wait queue.
+ */
+ smp_mb();
+ wake_up_bit(&inode->i_state, __I_WB_SWITCH);
+
if (switched) {
wb_wakeup(new_wb);
wb_put(old_wb);
}
- wb_put(new_wb);
+}
- iput(inode);
- kfree(isw);
+static void inode_switch_wbs_work_fn(struct work_struct *work)
+{
+ struct inode_switch_wbs_context *isw =
+ container_of(work, struct inode_switch_wbs_context, work);
+ do_inode_switch_wbs(isw->inode, isw->new_wb);
+ wb_put(isw->new_wb);
+ iput(isw->inode);
+ kfree(isw);
atomic_dec(&isw_nr_in_flight);
}
@@ -521,6 +532,39 @@ static void inode_switch_wbs(struct inode *inode, int new_wb_id)
}
/**
+ * inode_switch_to_default_wb_sync - change the wb association of an inode to
+ * the default writeback structure synchronously
+ * @inode: target inode
+ *
+ * Switch @inode's wb association to the default writeback structure (bdi->wb).
+ * Unlike inode_switch_wbs() the switching is performed synchronously and we
+ * guarantee the inode is switched to the default writeback structure when this
+ * function returns. Nothing prevents from someone else switching inode to
+ * another writeback structure just when we are done though. Preventing that is
+ * upto the caller if needed.
+ */
+void inode_switch_to_default_wb_sync(struct inode *inode)
+{
+ struct backing_dev_info *bdi = inode_to_bdi(inode);
+
+ /* while holding I_WB_SWITCH, no one else can update the association */
+ spin_lock(&inode->i_lock);
+ if (WARN_ON_ONCE(inode->i_state & I_FREEING) ||
+ !inode_to_wb_is_valid(inode) || inode_to_wb(inode) == &bdi->wb) {
+ spin_unlock(&inode->i_lock);
+ return;
+ }
+ __inode_wait_for_state_bit(inode, __I_WB_SWITCH);
+ inode->i_state |= I_WB_SWITCH;
+ spin_unlock(&inode->i_lock);
+
+ /* Make I_WB_SWITCH setting visible to unlocked users of i_wb */
+ synchronize_rcu();
+
+ do_inode_switch_wbs(inode, &bdi->wb);
+}
+
+/**
* wbc_attach_and_unlock_inode - associate wbc with target inode and unlock it
* @wbc: writeback_control of interest
* @inode: target inode
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c930cbc19342..319fb76f9081 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1929,7 +1929,8 @@ static inline bool HAS_UNMAPPED_ID(struct inode *inode)
#define I_DIRTY_TIME (1 << 11)
#define __I_DIRTY_TIME_EXPIRED 12
#define I_DIRTY_TIME_EXPIRED (1 << __I_DIRTY_TIME_EXPIRED)
-#define I_WB_SWITCH (1 << 13)
+#define __I_WB_SWITCH 13
+#define I_WB_SWITCH (1 << __I_WB_SWITCH)
#define I_DIRTY (I_DIRTY_SYNC | I_DIRTY_DATASYNC | I_DIRTY_PAGES)
#define I_DIRTY_ALL (I_DIRTY | I_DIRTY_TIME)
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 5527d910ba3d..0d3ba83a0f7f 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -280,6 +280,8 @@ static inline void wbc_init_bio(struct writeback_control *wbc, struct bio *bio)
bio_associate_blkcg(bio, wbc->wb->blkcg_css);
}
+void inode_switch_to_default_wb_sync(struct inode *inode);
+
#else /* CONFIG_CGROUP_WRITEBACK */
static inline void inode_attach_wb(struct inode *inode, struct page *page)
@@ -319,6 +321,10 @@ static inline void cgroup_writeback_umount(void)
{
}
+static inline void inode_switch_to_default_wb_sync(struct inode *inode)
+{
+}
+
#endif /* CONFIG_CGROUP_WRITEBACK */
/*
--
2.10.2
^ permalink raw reply related
* [PATCH 08/10] block: Fix oops in locked_inode_to_wb_and_lock_list()
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
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.
Also add a warning to catch if someone uses inode_detach_wb() in a
dangerous way.
Reported-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
block/genhd.c | 4 ++--
fs/block_dev.c | 11 ++++-------
include/linux/fs.h | 2 +-
include/linux/writeback.h | 1 +
4 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 68c613edb93a..721921a140cc 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -649,13 +649,13 @@ void del_gendisk(struct gendisk *disk)
DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
while ((part = disk_part_iter_next(&piter))) {
invalidate_partition(disk, part->partno);
- bdev_unhash_inode(part_devt(part));
+ bdev_cleanup_inode(part_devt(part));
delete_partition(disk, part->partno);
}
disk_part_iter_exit(&piter);
invalidate_partition(disk, 0);
- bdev_unhash_inode(disk_devt(disk));
+ bdev_cleanup_inode(disk_devt(disk));
set_capacity(disk, 0);
disk->flags &= ~GENHD_FL_UP;
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 360439373a66..65ac3a60ac8e 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -884,6 +884,8 @@ static void bdev_evict_inode(struct inode *inode)
spin_lock(&bdev_lock);
list_del_init(&bdev->bd_list);
spin_unlock(&bdev_lock);
+ /* Detach inode from wb early as bdi_put() may free bdi->wb */
+ inode_detach_wb(inode);
if (bdev->bd_bdi != &noop_backing_dev_info)
bdi_put(bdev->bd_bdi);
}
@@ -960,13 +962,14 @@ static LIST_HEAD(all_bdevs);
* If there is a bdev inode for this device, unhash it so that it gets evicted
* as soon as last inode reference is dropped.
*/
-void bdev_unhash_inode(dev_t dev)
+void bdev_cleanup_inode(dev_t dev)
{
struct inode *inode;
inode = ilookup5(blockdev_superblock, hash(dev), bdev_test, &dev);
if (inode) {
remove_inode_hash(inode);
+ inode_switch_to_default_wb_sync(inode);
iput(inode);
}
}
@@ -1874,12 +1877,6 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
kill_bdev(bdev);
bdev_write_inode(bdev);
- /*
- * Detaching bdev inode from its wb in __destroy_inode()
- * is too late: the queue which embeds its bdi (along with
- * root wb) can be gone as soon as we put_disk() below.
- */
- inode_detach_wb(bdev->bd_inode);
}
if (bdev->bd_contains == bdev) {
if (disk->fops->release)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 319fb76f9081..f8c86b9c31d5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2344,7 +2344,7 @@ extern struct kmem_cache *names_cachep;
#ifdef CONFIG_BLOCK
extern int register_blkdev(unsigned int, const char *);
extern void unregister_blkdev(unsigned int, const char *);
-extern void bdev_unhash_inode(dev_t dev);
+extern void bdev_cleanup_inode(dev_t dev);
extern struct block_device *bdget(dev_t);
extern struct block_device *bdgrab(struct block_device *bdev);
extern void bd_set_size(struct block_device *, loff_t size);
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 0d3ba83a0f7f..6d27b78c9a79 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -237,6 +237,7 @@ static inline void inode_attach_wb(struct inode *inode, struct page *page)
static inline void inode_detach_wb(struct inode *inode)
{
if (inode->i_wb) {
+ WARN_ON_ONCE(!(inode->i_state & I_CLEAR));
wb_put(inode->i_wb);
inode->i_wb = NULL;
}
--
2.10.2
^ permalink raw reply related
* [PATCH 09/10] kobject: Export kobject_get_unless_zero()
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
Make the function available for outside use and fortify it against NULL
kobject.
Signed-off-by: Jan Kara <jack@suse.cz>
---
include/linux/kobject.h | 2 ++
lib/kobject.c | 5 ++++-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index e6284591599e..ca85cb80e99a 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -108,6 +108,8 @@ extern int __must_check kobject_rename(struct kobject *, const char *new_name);
extern int __must_check kobject_move(struct kobject *, struct kobject *);
extern struct kobject *kobject_get(struct kobject *kobj);
+extern struct kobject * __must_check kobject_get_unless_zero(
+ struct kobject *kobj);
extern void kobject_put(struct kobject *kobj);
extern const void *kobject_namespace(struct kobject *kobj);
diff --git a/lib/kobject.c b/lib/kobject.c
index 445dcaeb0f56..763d70a18941 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -601,12 +601,15 @@ struct kobject *kobject_get(struct kobject *kobj)
}
EXPORT_SYMBOL(kobject_get);
-static struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj)
+struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj)
{
+ if (!kobj)
+ return NULL;
if (!kref_get_unless_zero(&kobj->kref))
kobj = NULL;
return kobj;
}
+EXPORT_SYMBOL(kobject_get_unless_zero);
/*
* kobject_cleanup - free kobject resources.
--
2.10.2
^ permalink raw reply related
* [PATCH 10/10] block: Fix oops scsi_disk_get()
From: Jan Kara @ 2017-02-09 12:44 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-1-jack@suse.cz>
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>
---
block/genhd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/genhd.c b/block/genhd.c
index 721921a140cc..e3dbc311c323 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1350,7 +1350,7 @@ struct kobject *get_disk(struct gendisk *disk)
owner = disk->fops->owner;
if (owner && !try_module_get(owner))
return NULL;
- kobj = kobject_get(&disk_to_dev(disk)->kobj);
+ kobj = kobject_get_unless_zero(&disk_to_dev(disk)->kobj);
if (kobj == NULL) {
module_put(owner);
return NULL;
--
2.10.2
^ permalink raw reply related
* Re: Boot regression (was "Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements")
From: hch @ 2017-02-09 13:08 UTC (permalink / raw)
To: Dexuan Cui
Cc: Jens Axboe, Bart Van Assche, hare@suse.com, hare@suse.de,
Martin K. Petersen, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org, jth@kernel.org
In-Reply-To: <MWHPR03MB26698583CE04682F588ACAD4BF450@MWHPR03MB2669.namprd03.prod.outlook.com>
Hi Dexuan,
I've spent some time with the logs and looking over the code and
couldn't find any smoking gun. I start to wonder if it might just
be a timing issue?
Can you try one or two things for me:
1) run with the blk-mq I/O path for scsi by either enabling it a boot /
module load time with the scsi_mod.use_blk_mq=Y option, or at compile
time by enabling the CONFIG_SCSI_MQ_DEFAULT option. If that fails
with the commit a blk-mq run before the commit would also be useful.
2) if possible run a VM config without the virtual CD-ROM drive -
a lot of the scsi log chatter is about handling timeouts on the
CD drive, so that might be able to isolate issues a bit better.
Note that I'll be offline from this afternoon European time until Sunday
night as I'm out in the mountains at a lodge without internet access,
but this issue will be my priority once back.
^ permalink raw reply
* Re: automatic IRQ affinity for virtio V3
From: Christoph Hellwig @ 2017-02-09 13:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: mst, jasowang, axboe, pbonzini, virtualization, linux-block,
linux-kernel
In-Reply-To: <20170205171526.6224-1-hch@lst.de>
On Sun, Feb 05, 2017 at 06:15:17PM +0100, Christoph Hellwig wrote:
> Hi Michael, hi Jason,
>
> This patches applies a few cleanups to the virtio PCI interrupt handling
> code, and then converts the virtio PCI code to use the automatic MSI-X
> vectors spreading, as well as using the information in virtio-blk
> and virtio-scsi to automatically align the blk-mq queues to the MSI-X
> vectors.
Any chance to get this in for 4.11 after I got reviews from Jason
for most of the patches?
^ permalink raw reply
* Re: [PATCH 16/24] exofs: Convert to separately allocated bdi
From: Boaz Harrosh @ 2017-02-09 14:23 UTC (permalink / raw)
To: Jan Kara, linux-fsdevel
Cc: Christoph Hellwig, linux-block, Benny Halevy, osd-dev
In-Reply-To: <20170202173422.3240-17-jack@suse.cz>
On 02/02/2017 07:34 PM, Jan Kara wrote:
> Allocate struct backing_dev_info separately instead of embedding it
> inside the superblock. This unifies handling of bdi among users.
>
> CC: Boaz Harrosh <ooo@electrozaur.com>
> CC: Benny Halevy <bhalevy@primarydata.com>
> CC: osd-dev@open-osd.org
ACK-by: Boaz Harrosh <ooo@electrozaur.com>
Looks fine thanks
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/exofs/exofs.h | 1 -
> fs/exofs/super.c | 17 ++++++-----------
> 2 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/fs/exofs/exofs.h b/fs/exofs/exofs.h
> index 2e86086bc940..5dc392404559 100644
> --- a/fs/exofs/exofs.h
> +++ b/fs/exofs/exofs.h
> @@ -64,7 +64,6 @@ struct exofs_dev {
> * our extension to the in-memory superblock
> */
> struct exofs_sb_info {
> - struct backing_dev_info bdi; /* register our bdi with VFS */
> struct exofs_sb_stats s_ess; /* Written often, pre-allocate*/
> int s_timeout; /* timeout for OSD operations */
> uint64_t s_nextid; /* highest object ID used */
> diff --git a/fs/exofs/super.c b/fs/exofs/super.c
> index 1076a4233b39..819624cfc8da 100644
> --- a/fs/exofs/super.c
> +++ b/fs/exofs/super.c
> @@ -464,7 +464,6 @@ static void exofs_put_super(struct super_block *sb)
> sbi->one_comp.obj.partition);
>
> exofs_sysfs_sb_del(sbi);
> - bdi_destroy(&sbi->bdi);
> exofs_free_sbi(sbi);
> sb->s_fs_info = NULL;
> }
> @@ -809,8 +808,12 @@ static int exofs_fill_super(struct super_block *sb, void *data, int silent)
> __sbi_read_stats(sbi);
>
> /* set up operation vectors */
> - sbi->bdi.ra_pages = __ra_pages(&sbi->layout);
> - sb->s_bdi = &sbi->bdi;
> + ret = super_setup_bdi(sb);
> + if (ret) {
> + EXOFS_DBGMSG("Failed to super_setup_bdi\n");
> + goto free_sbi;
> + }
> + sb->s_bdi->ra_pages = __ra_pages(&sbi->layout);
> sb->s_fs_info = sbi;
> sb->s_op = &exofs_sops;
> sb->s_export_op = &exofs_export_ops;
> @@ -836,14 +839,6 @@ static int exofs_fill_super(struct super_block *sb, void *data, int silent)
> goto free_sbi;
> }
>
> - ret = bdi_setup_and_register(&sbi->bdi, "exofs");
> - if (ret) {
> - EXOFS_DBGMSG("Failed to bdi_setup_and_register\n");
> - dput(sb->s_root);
> - sb->s_root = NULL;
> - goto free_sbi;
> - }
> -
> exofs_sysfs_dbg_print();
> _exofs_print_device("Mounting", opts->dev_name,
> ore_comp_dev(&sbi->oc, 0),
>
^ permalink raw reply
* RE: [PATCH] block: sed-opal: reduce stack size of ioctl handler
From: David Laight @ 2017-02-09 14:26 UTC (permalink / raw)
To: 'Arnd Bergmann', Scott Bauer, Jonathan Derrick,
Rafael Antognolli
Cc: Jens Axboe, linux-kernel@vger.kernel.org,
linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
Paul Mackerras, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20170208211546.2789607-1-arnd@arndb.de>
From: Arnd Bergmann
> Sent: 08 February 2017 21:15
>
> When CONFIG_KASAN is in use, the sed_ioctl function uses unusually large =
stack,
> as each possible ioctl argument gets its own stack area plus redzone:
Why not do a single copy_from_user() at the top of sed_ioctl() based on
the _IOC_DIR() and __IOC_SIZE() values?
Something like:
int sed_ioctl(..., unsigned int cmd, void __user *arg)
{
u64 buf[??]; /* or a union */
unsigned int cmd_sz =3D _IOC_SIZE(cmd);
if (_IOC_DIR(cmd) & (_IOC_WRITE | _IOC_READ) && cmd_sz > sizeof buf)
return -EINVAL;
if (_IOC_DIR(cmd) & _IOC_WRITE) {
if (copy_from_user(buf, arg, cmd_sz))
return -EFAULT;
} else {
if (IOC_DIR(cmd) & _IOC_READ))
memzero(buf, cmd_sz);
}
switch (cmd) {
...
rval =3D ...
...
}
if (rval >=3D 0 && (_IOC_DIR(cmd) & _IOC_READ)
&& copy_to_user(arg, buf, cmd_sz));
return -EFAULT;
return rval;
}
David
^ permalink raw reply
* Re: [PATCH 05/24] fs: Get proper reference for s_bdi
From: Boaz Harrosh @ 2017-02-09 14:36 UTC (permalink / raw)
To: Jan Kara, linux-fsdevel; +Cc: Christoph Hellwig, linux-block
In-Reply-To: <20170202173422.3240-6-jack@suse.cz>
On 02/02/2017 07:34 PM, Jan Kara wrote:
> So far we just relied on block device to hold a bdi reference for us
> while the filesystem is mounted. While that works perfectly fine, it is
> a bit awkward that we have a pointer to a refcounted structure in the
> superblock without proper reference. So make s_bdi hold a proper
> reference to block device's BDI. No filesystem using mount_bdev()
> actually changes s_bdi so this is safe and will make bdev filesystems
> work the same way as filesystems needing to set up their private bdi.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/super.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 31dc4c6450ef..dfb95ccd4351 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1047,12 +1047,9 @@ static int set_bdev_super(struct super_block *s, void *data)
> {
> s->s_bdev = data;
> s->s_dev = s->s_bdev->bd_dev;
> + s->s_bdi = bdi_get(s->s_bdev->bd_bdi);
> + s->s_iflags |= SB_I_DYNBDI;
>
> - /*
> - * We set the bdi here to the queue backing, file systems can
> - * overwrite this in ->fill_super()
> - */
Question: So I have an FS that uses mount_bdev but than goes and overrides sb->s_bdev
in ->fill_super() anyway. This is because of two reasons. One because I have many
more devices. (like btrfs I'm moulti-dev) but I like to use mount_bdev because of the
somewhat delicate handling of automatic bind-mounts.
For me it is a bigger hack to get the ref-counting and bind-mounts locking correctly
then to bdi_put and say the new super_setup_bdi(sb) in fill_super. Would you expect
problems?
Thanks for any help
Boaz
> - s->s_bdi = bdev_get_queue(s->s_bdev)->backing_dev_info;
> return 0;
> }
>
>
^ 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