* Sed-opal Fixes
From: Scott Bauer @ 2017-02-10 16:00 UTC (permalink / raw)
To: linux-nvme
Cc: keith.busch, arnd, hch, linux-kernel, axboe, David.Laight,
linux-block, jonathan.derrick
changes from v3->v4:
Changed manual kzalloc + copy from user to memdup_user.
This small series fixes a small ABI issue when using the _IOC_SIZE
with the Activate Locking SP ioctl. I had put the wrong structure
in the IOW macro in the uapi header which caused issues when trying
to copy in the contents for the command.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Scott Bauer @ 2017-02-10 15:57 UTC (permalink / raw)
To: Arnd Bergmann
Cc: keith.busch, hch, linux-kernel, linux-nvme, axboe, David.Laight,
linux-block, jonathan.derrick
In-Reply-To: <4455227.qpXP0b5apU@wuerfel>
On Fri, Feb 10, 2017 at 09:01:23AM +0100, Arnd Bergmann wrote:
> On Thursday, February 9, 2017 10:20:01 AM CET Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:
> >
> > block/sed-opal.c: In function 'sed_ioctl':
> > block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
> >
> > Moved all the ioctl structures off the stack and dynamically activate
> > using _IOC_SIZE()
> >
> > Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> >
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Scott Bauer <scott.bauer@intel.com>
> > ---
> > block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
> > 1 file changed, 50 insertions(+), 84 deletions(-)
> >
> > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > index bf1406e..4985d95 100644
> > --- a/block/sed-opal.c
> > +++ b/block/sed-opal.c
> > @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
> >
> > 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;
> > + unsigned int cmd_size = _IOC_SIZE(cmd);
> >
> > if (!capable(CAP_SYS_ADMIN))
> > return -EACCES;
>
> We usually have a size check in there to avoid allocating large amounts
> of memory. _IOC_SIZEBITS is 14, so you can have up to 16kb here, which
> is probably ok, but I'd recommend either adding a comment to say that
> it is, or just checking against the largest realistic size.
Right now it's up to the storage driver to call into the sed-opal ioctl.
We have a function is_sed_ioctl() which is called before jumping into sed_ioctl.
So we will be weeding out any non opal ioctls before getting in there so I don't
see any overly large 16kb allocations happening.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCHv6 11/37] HACK: readahead: alloc huge pages, if allowed
From: Matthew Wilcox @ 2017-02-10 14:51 UTC (permalink / raw)
To: Andreas Dilger
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: <7D35EB8E-29F8-41DA-BB46-8BCF7B6C5A72@dilger.ca>
On Thu, Feb 09, 2017 at 05:23:31PM -0700, Andreas Dilger wrote:
> On Feb 9, 2017, at 4:34 PM, Matthew Wilcox <willy@infradead.org> wrote:
> > Well ... what if we made readahead 2 hugepages in size for inodes which
> > are using huge pages? That's only 8x our current readahead window, and
> > if you're asking for hugepages, you're accepting that IOs are going to
> > be larger, and you probably have the kind of storage system which can
> > handle doing larger IOs.
>
> It would be nice if the bdi had a parameter for the maximum readahead size.
> Currently, readahead is capped at 2MB chunks by force_page_cache_readahead()
> even if bdi->ra_pages and bdi->io_pages are much larger.
>
> It should be up to the filesystem to decide how large the readahead chunks
> are rather than imposing some policy in the MM code. For high-speed (network)
> storage access it is better to have at least 4MB read chunks, for RAID storage
> it is desirable to have stripe-aligned readahead to avoid read inflation when
> verifying the parity. Any fixed size will eventually be inadequate as disks
> and filesystems change, so it may as well be a per-bdi tunable that can be set
> by the filesystem as needed, or possibly with a mount option if needed.
I think the filesystem should provide a hint, but ultimately it needs to
be up to the MM to decide how far to readahead. The filesystem doesn't
(and shouldn't) have the global view into how much memory is available
for readahead, nor should it be tracking how well this app is being
served by readahead.
That 2MB chunk restriction is allegedly there "so that we don't pin too
much memory at once". Maybe that should be scaled with the amount of
memory in the system (pinning 2MB of a 256MB system is a bit different
from pinning 2MB of a 1TB memory system).
^ permalink raw reply
* RE: Boot regression (was "Re: [PATCH] genhd: Do not hold event lock when scheduling workqueue elements")
From: Dexuan Cui @ 2017-02-10 14:49 UTC (permalink / raw)
To: hch@lst.de
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, Christoph Hellwig,
Nick Meier, Alex Ng (LIS), Long Li,
Adrian Suhov (Cloudbase Solutions SRL),
Chris Valean (Cloudbase Solutions SRL)
In-Reply-To: <20170209130800.GA12057@lst.de>
[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]
> From: hch@lst.de [mailto:hch@lst.de]
>
> 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?
Thanks a lot for digging into the issue!
I'm not sure if it's a timing issue, but the fact is that the VM hangs every time
due to the issue, when it tries to boot up.
We tried both UP VM and SMP VM, and they always failed to boot up.
> Can you try one or two things for me:
I did the following tests with
1 CPU,
IDE controller 0 with the VM's image,
IDE controller 1 with no disk,
no SCSI controller, and
no CD drive.
> 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.
With the scsi_mod.use_blk_mq=Y kernel parameter, the VM can boot fine!
And with =N (the default), the VM hits the issue.
> 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.
Please see the attached logs.
I got the good.log by reverting the bad commit. It's so long so I only
reserve the first part of the log.
> 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.
Have a nice trip! :-)
Thanks,
-- Dexuan
[-- Attachment #2: good-bad-scsi-logs.zip --]
[-- Type: application/x-zip-compressed, Size: 80096 bytes --]
^ permalink raw reply
* Re: [PATCH 07/10] writeback: Implement reliable switching to default writeback structure
From: Jan Kara @ 2017-02-10 13:20 UTC (permalink / raw)
To: NeilBrown
Cc: Jan Kara, Jens Axboe, linux-block, Christoph Hellwig, Tejun Heo,
Dan Williams, Thiago Jung Bauermann, NeilBrown
In-Reply-To: <87zihuu6rj.fsf@notabene.neil.brown.name>
On Fri 10-02-17 13:19:44, NeilBrown wrote:
> On Thu, Feb 09 2017, Jan Kara wrote:
>
> > 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.
>
> I think you mean "set_current_state()" ??
Yes, I'll fix that.
> It's rather a trap for the unwary, this need for a smp_mb().
> Greping for wake_up_bit(), I find quite a few places with barriers -
> sometimes clear_bit_unlock() or spin_unlock() - but
>
> fs/block_dev.c- whole->bd_claiming = NULL;
> fs/block_dev.c: wake_up_bit(&whole->bd_claiming, 0);
>
> fs/cifs/connect.c- clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
> fs/cifs/connect.c: wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
>
> fs/cifs/misc.c- clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
> fs/cifs/misc.c: wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
>
> (several more in cifs)
>
> net/sunrpc/xprt.c- clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
> net/sunrpc/xprt.c- xprt->ops->close(xprt);
> net/sunrpc/xprt.c- xprt_release_write(xprt, NULL);
> net/sunrpc/xprt.c: wake_up_bit(&xprt->state, XPRT_LOCKED);
> (there might be a barrier in ->close or xprt_release_write() I guess)
>
> security/keys/gc.c- clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
> security/keys/gc.c: wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
Yup, the above look like bugs.
> I wonder if there is a good way to make this less error-prone.
> I would suggest that wake_up_bit() should always have a barrier, and
> __wake_up_bit() is needed to avoid it, but there is already a
> __wake_up_bit() with a slightly different interface.
Yeah, it is error-prone as all waitqueue_active() optimizations...
> In this case, you have a spin_unlock() just before the wake_up_bit().
> It is my understand that it would provide enough of a barrier (all
> writes before are globally visible after), so do you really need
> the barrier here?
I believe I do. spin_unlock() is a semi-permeable barrier - i.e., any read
or write from "outside" can be moved inside. So CPU is free to prefetch
values for waitqueue active checks before the spinlock is unlocked or even
before clearing I_WB_SWITCH bit.
> > + */
> > + 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);
>
> I note that __inode_wait_for_state_bit() can drop and reclaim ->i_lock.
> is it possible that:
> !inode_to_wb_is_valid(inode) || inode_to_wb(inode) == &bdi->wb)
>
> could change while ->i_lock is unlocked?
> It would be particular unfortunate if inode_to_wb(inode) became &bdi->wb
> due to some thing thread, as do_inode_switch_wbs() will deadlock if
> inode_to_wb(inode) == &bdi->wb
>
> i.e. do you need to repeat the test?
That's a very good question and I think you are right that I need to repeat
the checks for inode->i_wb. Will fix. Thanks for review!
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] bfq-mq: cause deadlock by executing exit_icq body immediately
From: Paolo Valente @ 2017-02-10 13:00 UTC (permalink / raw)
To: Omar Sandoval
Cc: Jens Axboe, Tejun Heo, linux-block, Linux-Kernal, Ulf Hansson,
Linus Walleij, broonie
In-Reply-To: <20170208171713.GA7811@vader>
> Il giorno 08 feb 2017, alle ore 18:17, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>=20
> On Wed, Feb 08, 2017 at 11:39:24AM +0100, Paolo Valente wrote:
>>=20
>>> Il giorno 08 feb 2017, alle ore 11:33, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>>>=20
>>> On Wed, Feb 08, 2017 at 11:03:01AM +0100, Paolo Valente wrote:
>>>>=20
>>>>> Il giorno 07 feb 2017, alle ore 22:45, Omar Sandoval =
<osandov@osandov.com> ha scritto:
>>>>>=20
>>>>> On Tue, Feb 07, 2017 at 06:33:46PM +0100, Paolo Valente wrote:
>>>>>> Hi,
>>>>>> this patch is meant to show that, if the body of the hook =
exit_icq is executed
>>>>>> from inside that hook, and not as deferred work, then a circular =
deadlock
>>>>>> occurs.
>>>>>>=20
>>>>>> It happens if, on a CPU
>>>>>> - the body of icq_exit takes the scheduler lock,
>>>>>> - it does so from inside the exit_icq hook, which is invoked with =
the queue
>>>>>> lock held
>>>>>>=20
>>>>>> while, on another CPU
>>>>>> - bfq_bio_merge, after taking the scheduler lock, invokes =
bfq_bic_lookup,
>>>>>> which, in its turn, takes the queue lock. bfq_bic_lookup needs to =
take such a
>>>>>> lock, because it invokes ioc_lookup_icq.
>>>>>>=20
>>>>>> For more details, here is a lockdep report, right before the =
deadlock did occur.
>>>>>>=20
>>>>>> [ 44.059877] =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
>>>>>> [ 44.124922] [ INFO: possible circular locking dependency =
detected ]
>>>>>> [ 44.125795] 4.10.0-rc5-bfq-mq+ #38 Not tainted
>>>>>> [ 44.126414] =
-------------------------------------------------------
>>>>>> [ 44.127291] sync/2043 is trying to acquire lock:
>>>>>> [ 44.128918] (&(&bfqd->lock)->rlock){-.-...}, at: =
[<ffffffff90484195>] bfq_exit_icq_bfqq+0x55/0x140
>>>>>> [ 44.134052]
>>>>>> [ 44.134052] but task is already holding lock:
>>>>>> [ 44.134868] (&(&q->__queue_lock)->rlock){-.....}, at: =
[<ffffffff9044738e>] put_io_context_active+0x6e/0xc0
>>>>>=20
>>>>> Hey, Paolo,
>>>>>=20
>>>>> I only briefly skimmed the code, but what are you using the =
queue_lock
>>>>> for? You should just use your scheduler lock everywhere. blk-mq =
doesn't
>>>>> use the queue lock, so the scheduler is the only thing you need =
mutual
>>>>> exclusion against.
>>>>=20
>>>> Hi Omar,
>>>> the cause of the problem is that the hook functions =
bfq_request_merge
>>>> and bfq_allow_bio_merge invoke, directly or through other =
functions,
>>>> the function bfq_bic_lookup, which, in its turn, invokes
>>>> ioc_lookup_icq. The latter must be invoked with the queue lock =
held.
>>>> In particular the offending lines in bfq_bic_lookup are:
>>>>=20
>>>> spin_lock_irqsave(q->queue_lock, flags);
>>>> icq =3D icq_to_bic(ioc_lookup_icq(ioc, q));
>>>> spin_unlock_irqrestore(q->queue_lock, flags);
>>>>=20
>>>> Maybe I'm missing something and we can avoid taking this lock?
>>>=20
>>> Ah, I didn't realize we still used the q->queue_lock for the icq =
stuff.
>>> You're right, you still need that lock for ioc_lookup_icq(). Unless
>>> there's something else I'm forgetting, that should be the only thing =
you
>>> need it for in the core code, and you should use your scheduler lock =
for
>>> everything else. What else are you using q->queue_lock for?=20
>>=20
>> Nothing. The deadlock follows from that bfq_request_merge gets =
called
>> with the scheduler lock already held. Problematic paths start from:
>> bfq_bio_merge and bfq_insert_request.
>>=20
>> I'm trying to understand whether I/we can reorder operations in some
>> way that avoids the nested locking, but at no avail so far.
>>=20
>> Thanks,
>> Paolo
>=20
> Okay, I understand what you're saying now. It was all in the first =
email
> but I didn't see it right away, sorry about that.
>=20
> I don't think it makes sense for ->exit_icq() to be invoked while
> holding q->queue_lock for blk-mq -- we don't hold that lock for any of
> the other hooks. Could you try the below? I haven't convinced myself
> that there isn't a circular locking dependency between bfqd->lock and
> ioc->lock now, but it's probably easiest for you to just try it.
>=20
Just passed the last of a heavy batch of tests!
I have updated the bfq-mq branch [1], adding a temporary commit that
contains your diffs (while waiting for you final patch or the like).
Looking forward to some feedback on the other issue I have raised on
locking, and to some review of the current version of bfq-mq, before
proceeding with cgroups support.
Thanks,
Paolo
[1] https://github.com/Algodev-github/bfq-mq
> diff --git a/block/blk-ioc.c b/block/blk-ioc.c
> index fe186a9eade9..b12f9c87b4c3 100644
> --- a/block/blk-ioc.c
> +++ b/block/blk-ioc.c
> @@ -35,7 +35,10 @@ static void icq_free_icq_rcu(struct rcu_head *head)
> kmem_cache_free(icq->__rcu_icq_cache, icq);
> }
>=20
> -/* Exit an icq. Called with both ioc and q locked. */
> +/*
> + * Exit an icq. Called with both ioc and q locked for sq, only ioc =
locked for
> + * mq.
> + */
> static void ioc_exit_icq(struct io_cq *icq)
> {
> struct elevator_type *et =3D icq->q->elevator->type;
> @@ -166,6 +169,7 @@ EXPORT_SYMBOL(put_io_context);
> */
> void put_io_context_active(struct io_context *ioc)
> {
> + struct elevator_type *et;
> unsigned long flags;
> struct io_cq *icq;
>=20
> @@ -184,13 +188,19 @@ void put_io_context_active(struct io_context =
*ioc)
> hlist_for_each_entry(icq, &ioc->icq_list, ioc_node) {
> if (icq->flags & ICQ_EXITED)
> continue;
> - if (spin_trylock(icq->q->queue_lock)) {
> +
> + et =3D icq->q->elevator->type;
> + if (et->uses_mq) {
> ioc_exit_icq(icq);
> - spin_unlock(icq->q->queue_lock);
> } else {
> - spin_unlock_irqrestore(&ioc->lock, flags);
> - cpu_relax();
> - goto retry;
> + if (spin_trylock(icq->q->queue_lock)) {
> + ioc_exit_icq(icq);
> + spin_unlock(icq->q->queue_lock);
> + } else {
> + spin_unlock_irqrestore(&ioc->lock, =
flags);
> + cpu_relax();
> + goto retry;
> + }
> }
> }
> spin_unlock_irqrestore(&ioc->lock, flags);
^ permalink raw reply
* Re: [PATCH 3/6] genirq/affinity: update CPU affinity for CPU hotplug events
From: Thomas Gleixner @ 2017-02-10 11:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Keith Busch, linux-nvme, linux-block, linux-kernel
In-Reply-To: <20170203143600.32307-4-hch@lst.de>
On Fri, 3 Feb 2017, Christoph Hellwig wrote:
> @@ -127,6 +127,7 @@ enum cpuhp_state {
> CPUHP_AP_ONLINE_IDLE,
> CPUHP_AP_SMPBOOT_THREADS,
> CPUHP_AP_X86_VDSO_VMA_ONLINE,
> + CPUHP_AP_IRQ_AFFINIY_ONLINE,
s/AFFINIY/AFFINITY/ perhaps?
> +static void __irq_affinity_set(unsigned int irq, struct irq_desc *desc,
> + cpumask_t *mask)
static void __irq_affinity_set(unsigned int irq, struct irq_desc *desc,
cpumask_t *mask)
Please
> +{
> + struct irq_data *data = irq_desc_get_irq_data(desc);
> + struct irq_chip *chip = irq_data_get_irq_chip(data);
> + int ret;
> +
> + if (!irqd_can_move_in_process_context(data) && chip->irq_mask)
> + chip->irq_mask(data);
> + ret = chip->irq_set_affinity(data, mask, true);
> + WARN_ON_ONCE(ret);
> +
> + /*
> + * We unmask if the irq was not marked masked by the core code.
> + * That respects the lazy irq disable behaviour.
> + */
> + if (!irqd_can_move_in_process_context(data) &&
> + !irqd_irq_masked(data) && chip->irq_unmask)
> + chip->irq_unmask(data);
> +}
This looks very familiar. arch/x86/kernel/irq.c comes to mind
> +
> +static void irq_affinity_online_irq(unsigned int irq, struct irq_desc *desc,
> + unsigned int cpu)
> +{
> + const struct cpumask *affinity;
> + struct irq_data *data;
> + struct irq_chip *chip;
> + unsigned long flags;
> + cpumask_var_t mask;
> +
> + if (!desc)
> + return;
> +
> + raw_spin_lock_irqsave(&desc->lock, flags);
> +
> + data = irq_desc_get_irq_data(desc);
> + affinity = irq_data_get_affinity_mask(data);
> + if (!irqd_affinity_is_managed(data) ||
> + !irq_has_action(irq) ||
> + !cpumask_test_cpu(cpu, affinity))
> + goto out_unlock;
> +
> + /*
> + * The interrupt descriptor might have been cleaned up
> + * already, but it is not yet removed from the radix tree
> + */
> + chip = irq_data_get_irq_chip(data);
> + if (!chip)
> + goto out_unlock;
> +
> + if (WARN_ON_ONCE(!chip->irq_set_affinity))
> + goto out_unlock;
> +
> + if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
You really want to allocate that _BEFORE_ locking desc->lock. GFP_KERNEL
inside the lock held region is wrong and shows that this was never tested :)
And no, we don't want GFP_ATOMIC here. You can allocate is once at the call
site and hand it in, so you avoid the alloc/free dance when iterating over
a large number of descriptors.
> + pr_err("failed to allocate memory for cpumask\n");
> + goto out_unlock;
> + }
> +
> + cpumask_and(mask, affinity, cpu_online_mask);
> + cpumask_set_cpu(cpu, mask);
> + if (irqd_has_set(data, IRQD_AFFINITY_SUSPENDED)) {
> + irq_startup(desc, false);
> + irqd_clear(data, IRQD_AFFINITY_SUSPENDED);
> + } else {
> + __irq_affinity_set(irq, desc, mask);
> + }
> +
> + free_cpumask_var(mask);
> +out_unlock:
> + raw_spin_unlock_irqrestore(&desc->lock, flags);
> +}
> +int irq_affinity_online_cpu(unsigned int cpu)
> +{
> + struct irq_desc *desc;
> + unsigned int irq;
> +
> + for_each_irq_desc(irq, desc)
> + irq_affinity_online_irq(irq, desc, cpu);
That lacks protection against concurrent irq setup/teardown. Wants to be
protected with irq_lock_sparse()
> + return 0;
> +}
> +
> +static void irq_affinity_offline_irq(unsigned int irq, struct irq_desc *desc,
> + unsigned int cpu)
> +{
> + const struct cpumask *affinity;
> + struct irq_data *data;
> + struct irq_chip *chip;
> + unsigned long flags;
> + cpumask_var_t mask;
> +
> + if (!desc)
> + return;
> +
> + raw_spin_lock_irqsave(&desc->lock, flags);
> +
> + data = irq_desc_get_irq_data(desc);
> + affinity = irq_data_get_affinity_mask(data);
> + if (!irqd_affinity_is_managed(data) ||
> + !irq_has_action(irq) ||
> + irqd_has_set(data, IRQD_AFFINITY_SUSPENDED) ||
> + !cpumask_test_cpu(cpu, affinity))
> + goto out_unlock;
> +
> + /*
> + * Complete the irq move. This cpu is going down and for
> + * non intr-remapping case, we can't wait till this interrupt
> + * arrives at this cpu before completing the irq move.
> + */
> + irq_force_complete_move(desc);
Hmm. That's what we do in x86 when the cpu is really dying, i.e. before it
really goes away. It's the last resort we have.
So if a move is pending, then you force it here and then you call
__irq_affinity_set() further down, which queues another pending move, which
then gets cleaned up in the cpu dying code.
If a move is pending, then you should first verify whether the pending
affinity mask is different from the one you are going to set. If it's the
same, you can just let the final cleanup code do its job. If not, then you
need to check whether it has something to do with the current affinity mask
or whether its completely different. Otherwise you just destroy the
previous setting which tried to move the interrupt to some other place
already.
> + /*
> + * The interrupt descriptor might have been cleaned up
> + * already, but it is not yet removed from the radix tree
> + */
> + chip = irq_data_get_irq_chip(data);
> + if (!chip)
> + goto out_unlock;
> +
> + if (WARN_ON_ONCE(!chip->irq_set_affinity))
> + goto out_unlock;
> +
> + if (!zalloc_cpumask_var(&mask, GFP_KERNEL)) {
> + pr_err("failed to allocate memory for cpumask\n");
> + goto out_unlock;
> + }
Same allocation issue.
> +
> + cpumask_copy(mask, affinity);
> + cpumask_clear_cpu(cpu, mask);
> + if (cpumask_empty(mask)) {
> + irqd_set(data, IRQD_AFFINITY_SUSPENDED);
> + irq_shutdown(desc);
> + } else {
> + __irq_affinity_set(irq, desc, mask);
> + }
> +
> + free_cpumask_var(mask);
> +out_unlock:
> + raw_spin_unlock_irqrestore(&desc->lock, flags);
> +}
> +
> +int irq_affinity_offline_cpu(unsigned int cpu)
> +{
> + struct irq_desc *desc;
> + unsigned int irq;
> +
> + for_each_irq_desc(irq, desc)
> + irq_affinity_offline_irq(irq, desc, cpu);
Same protection issue.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Arnd Bergmann @ 2017-02-10 11:00 UTC (permalink / raw)
To: David Laight
Cc: Johannes Thumshirn, Scott Bauer, linux-nvme@lists.infradead.org,
axboe@fb.com, keith.busch@intel.com, jonathan.derrick@intel.com,
hch@infradead.org, linux-kernel@vger.kernel.org,
linux-block@vger.kernel.org
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB02833C4@AcuExch.aculab.com>
On Fri, Feb 10, 2017 at 11:28 AM, David Laight <David.Laight@aculab.com> wrote:
>>
>> > - if (copy_from_user(&session, arg, sizeof(session)))
>> > - return -EFAULT;
>> > - return opal_erase_locking_range(dev, &session);
>> > + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
>> > + if (!ioctl_ptr)
>> > + return -ENOMEM;
>> > + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
>> > + ret = -EFAULT;
>> > + goto out;
>> > }
>>
>> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
>
> You either want the copy_from_user() or the memzero() not both.
>
> ISTM there could be two 'library' functions, maybe:
> void *get_ioctl_buf(unsigned int cmd, long arg)
> to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
> int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
> does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
> return value is rval unless the copyout fails.
All the ioctls commands in this driver are IOW, and no data is passed back
to user space, so there is no need for the memzero(): we can either copy
the data from user space or we fail.
Arnd
^ permalink raw reply
* [PATCH v1 0/5] md: use bio_clone_fast()
From: Ming Lei @ 2017-02-10 10:56 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
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.
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 v1 1/5] block: introduce bio_clone_bioset_partial()
From: Ming Lei @ 2017-02-10 10:56 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1486724177-14817-1-git-send-email-tom.leiming@gmail.com>
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.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
block/bio.c | 61 +++++++++++++++++++++++++++++++++++++++++------------
include/linux/bio.h | 11 ++++++++--
2 files changed, 57 insertions(+), 15 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 4b564d0c3e29..5eec5e08417f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -625,21 +625,20 @@ struct bio *bio_clone_fast(struct bio *bio, gfp_t gfp_mask, struct bio_set *bs)
}
EXPORT_SYMBOL(bio_clone_fast);
-/**
- * bio_clone_bioset - clone a bio
- * @bio_src: bio to clone
- * @gfp_mask: allocation priority
- * @bs: bio_set to allocate from
- *
- * Clone bio. Caller will own the returned bio, but not the actual data it
- * points to. Reference count of returned bio will be one.
- */
-struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
- struct bio_set *bs)
+static struct bio *__bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
+ struct bio_set *bs, int offset,
+ int size)
{
struct bvec_iter iter;
struct bio_vec bv;
struct bio *bio;
+ struct bvec_iter iter_src = bio_src->bi_iter;
+
+ /* for supporting partial clone */
+ if (offset || size != bio_src->bi_iter.bi_size) {
+ bio_advance_iter(bio_src, &iter_src, offset);
+ iter_src.bi_size = size;
+ }
/*
* Pre immutable biovecs, __bio_clone() used to just do a memcpy from
@@ -663,7 +662,8 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
* __bio_clone_fast() anyways.
*/
- bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
+ bio = bio_alloc_bioset(gfp_mask, __bio_segments(bio_src,
+ &iter_src), bs);
if (!bio)
return NULL;
bio->bi_bdev = bio_src->bi_bdev;
@@ -680,7 +680,7 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
bio->bi_io_vec[bio->bi_vcnt++] = bio_src->bi_io_vec[0];
break;
default:
- bio_for_each_segment(bv, bio_src, iter)
+ __bio_for_each_segment(bv, bio_src, iter, iter_src)
bio->bi_io_vec[bio->bi_vcnt++] = bv;
break;
}
@@ -699,9 +699,44 @@ struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
return bio;
}
+
+/**
+ * bio_clone_bioset - clone a bio
+ * @bio_src: bio to clone
+ * @gfp_mask: allocation priority
+ * @bs: bio_set to allocate from
+ *
+ * Clone bio. Caller will own the returned bio, but not the actual data it
+ * points to. Reference count of returned bio will be one.
+ */
+struct bio *bio_clone_bioset(struct bio *bio_src, gfp_t gfp_mask,
+ struct bio_set *bs)
+{
+ return __bio_clone_bioset(bio_src, gfp_mask, bs, 0,
+ bio_src->bi_iter.bi_size);
+}
EXPORT_SYMBOL(bio_clone_bioset);
/**
+ * bio_clone_bioset_partial - clone a partial bio
+ * @bio_src: bio to clone
+ * @gfp_mask: allocation priority
+ * @bs: bio_set to allocate from
+ * @offset: cloned starting from the offset
+ * @size: size for the cloned bio
+ *
+ * Clone bio. Caller will own the returned bio, but not the actual data it
+ * points to. Reference count of returned bio will be one.
+ */
+struct bio *bio_clone_bioset_partial(struct bio *bio_src, gfp_t gfp_mask,
+ struct bio_set *bs, int offset,
+ int size)
+{
+ return __bio_clone_bioset(bio_src, gfp_mask, bs, offset, size);
+}
+EXPORT_SYMBOL(bio_clone_bioset_partial);
+
+/**
* bio_add_pc_page - attempt to add page to bio
* @q: the target queue
* @bio: destination bio
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 7cf8a6c70a3f..8e521194f6fc 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -183,7 +183,7 @@ static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
#define bio_iter_last(bvec, iter) ((iter).bi_size == (bvec).bv_len)
-static inline unsigned bio_segments(struct bio *bio)
+static inline unsigned __bio_segments(struct bio *bio, struct bvec_iter *bvec)
{
unsigned segs = 0;
struct bio_vec bv;
@@ -205,12 +205,17 @@ static inline unsigned bio_segments(struct bio *bio)
break;
}
- bio_for_each_segment(bv, bio, iter)
+ __bio_for_each_segment(bv, bio, iter, *bvec)
segs++;
return segs;
}
+static inline unsigned bio_segments(struct bio *bio)
+{
+ return __bio_segments(bio, &bio->bi_iter);
+}
+
/*
* get a reference to a bio, so it won't disappear. the intended use is
* something like:
@@ -384,6 +389,8 @@ extern void bio_put(struct bio *);
extern void __bio_clone_fast(struct bio *, struct bio *);
extern struct bio *bio_clone_fast(struct bio *, gfp_t, struct bio_set *);
extern struct bio *bio_clone_bioset(struct bio *, gfp_t, struct bio_set *bs);
+extern struct bio *bio_clone_bioset_partial(struct bio *, gfp_t,
+ struct bio_set *, int, int);
extern struct bio_set *fs_bio_set;
--
2.7.4
^ permalink raw reply related
* [PATCH v1 3/5] md: fail if mddev->bio_set can't be created
From: Ming Lei @ 2017-02-10 10:56 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1486724177-14817-1-git-send-email-tom.leiming@gmail.com>
The current behaviour is to fall back to allocate
bio from 'fs_bio_set', that isn't a correct way
because it might cause deadlock.
So this patch simply return failure if mddev->bio_set
can't be created.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/md.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 4c1b82defa78..3425c2b779a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5270,8 +5270,11 @@ int md_run(struct mddev *mddev)
sysfs_notify_dirent_safe(rdev->sysfs_state);
}
- if (mddev->bio_set == NULL)
+ if (mddev->bio_set == NULL) {
mddev->bio_set = bioset_create(BIO_POOL_SIZE, 0);
+ if (!mddev->bio_set)
+ return -ENOMEM;
+ }
spin_lock(&pers_lock);
pers = find_pers(mddev->level, mddev->clevel);
--
2.7.4
^ permalink raw reply related
* [PATCH v1 2/5] md/raid1: use bio_clone_bioset_partial() in case of write behind
From: Ming Lei @ 2017-02-10 10:56 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1486724177-14817-1-git-send-email-tom.leiming@gmail.com>
Write behind need to replace pages in bio's bvecs, and we have
to clone a fresh bio with new bvec table, so use the introduced
bio_clone_bioset_partial() for it.
For other bio_clone_mddev() cases, we will use fast clone since
they don't need to touch bvec table.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/raid1.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 830ff2b20346..4d7852c6ae97 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1341,13 +1341,12 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
first_clone = 1;
for (i = 0; i < disks; i++) {
- struct bio *mbio;
+ 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;
if (first_clone) {
/* do behind I/O ?
@@ -1357,8 +1356,13 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
if (bitmap &&
(atomic_read(&bitmap->behind_writes)
< mddev->bitmap_info.max_write_behind) &&
- !waitqueue_active(&bitmap->behind_wait))
+ !waitqueue_active(&bitmap->behind_wait)) {
+ mbio = bio_clone_bioset_partial(bio, GFP_NOIO,
+ mddev->bio_set,
+ offset,
+ max_sectors);
alloc_behind_pages(mbio, r1_bio);
+ }
bitmap_startwrite(bitmap, r1_bio->sector,
r1_bio->sectors,
@@ -1366,6 +1370,12 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
&r1_bio->state));
first_clone = 0;
}
+
+ if (!mbio) {
+ mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ bio_trim(mbio, offset, max_sectors);
+ }
+
if (r1_bio->behind_bvecs) {
struct bio_vec *bvec;
int j;
--
2.7.4
^ permalink raw reply related
* [PATCH v1 5/5] md: fast clone bio in bio_clone_mddev()
From: Ming Lei @ 2017-02-10 10:56 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1486724177-14817-1-git-send-email-tom.leiming@gmail.com>
Firstly bio_clone_mddev() is used in raid normal I/O and isn't
in resync I/O path.
Secondly all the direct access to bvec table in raid happens on
resync I/O except for write behind of raid1, in which we still
use bio_clone() for allocating new bvec table.
So this patch replaces bio_clone() with bio_clone_fast()
in bio_clone_mddev().
Rename bio_clone_mddev() as bio_clone_fast_mddev() too, as
suggested by Christoph Hellwig.
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/faulty.c | 2 +-
drivers/md/md.c | 6 +++---
drivers/md/md.h | 4 ++--
drivers/md/raid1.c | 8 ++++----
drivers/md/raid10.c | 11 +++++------
drivers/md/raid5.c | 4 ++--
6 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/drivers/md/faulty.c b/drivers/md/faulty.c
index 685aa2d77e25..f80e7b8f8c40 100644
--- a/drivers/md/faulty.c
+++ b/drivers/md/faulty.c
@@ -214,7 +214,7 @@ static void faulty_make_request(struct mddev *mddev, struct bio *bio)
}
}
if (failit) {
- struct bio *b = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ struct bio *b = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
b->bi_bdev = conf->rdev->bdev;
b->bi_private = bio;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 2835f09b9e71..d45e8d1382ad 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -190,12 +190,12 @@ struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
}
EXPORT_SYMBOL_GPL(bio_alloc_mddev);
-struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
+struct bio *bio_clone_fast_mddev(struct bio *bio, gfp_t gfp_mask,
struct mddev *mddev)
{
- return bio_clone_bioset(bio, gfp_mask, mddev->bio_set);
+ return bio_clone_fast(bio, gfp_mask, mddev->bio_set);
}
-EXPORT_SYMBOL_GPL(bio_clone_mddev);
+EXPORT_SYMBOL_GPL(bio_clone_fast_mddev);
/*
* We have a system wide 'event count' that is incremented
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 968bbe72b237..88d0a101fb4c 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -673,8 +673,8 @@ extern void md_rdev_clear(struct md_rdev *rdev);
extern void mddev_suspend(struct mddev *mddev);
extern void mddev_resume(struct mddev *mddev);
-extern struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
- struct mddev *mddev);
+extern struct bio *bio_clone_fast_mddev(struct bio *bio, gfp_t gfp_mask,
+ struct mddev *mddev);
extern struct bio *bio_alloc_mddev(gfp_t gfp_mask, int nr_iovecs,
struct mddev *mddev);
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4d7852c6ae97..9e0b5a5ec0bc 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1108,7 +1108,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
r1_bio->read_disk = rdisk;
r1_bio->start_next_window = 0;
- read_bio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ read_bio = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
bio_trim(read_bio, r1_bio->sector - bio->bi_iter.bi_sector,
max_sectors);
@@ -1372,7 +1372,7 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
}
if (!mbio) {
- mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ mbio = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
bio_trim(mbio, offset, max_sectors);
}
@@ -2283,7 +2283,7 @@ static int narrow_write_error(struct r1bio *r1_bio, int i)
wbio->bi_vcnt = vcnt;
} else {
- wbio = bio_clone_mddev(r1_bio->master_bio, GFP_NOIO, mddev);
+ wbio = bio_clone_fast_mddev(r1_bio->master_bio, GFP_NOIO, mddev);
}
bio_set_op_attrs(wbio, REQ_OP_WRITE, 0);
@@ -2421,7 +2421,7 @@ static void handle_read_error(struct r1conf *conf, struct r1bio *r1_bio)
const unsigned long do_sync
= r1_bio->master_bio->bi_opf & REQ_SYNC;
r1_bio->read_disk = disk;
- bio = bio_clone_mddev(r1_bio->master_bio, GFP_NOIO, mddev);
+ bio = bio_clone_fast_mddev(r1_bio->master_bio, GFP_NOIO, mddev);
bio_trim(bio, r1_bio->sector - bio->bi_iter.bi_sector,
max_sectors);
r1_bio->bios[r1_bio->read_disk] = bio;
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 6bc5c2a85160..406d6651fd4c 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1132,7 +1132,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
}
slot = r10_bio->read_slot;
- read_bio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ read_bio = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
bio_trim(read_bio, r10_bio->sector - bio->bi_iter.bi_sector,
max_sectors);
@@ -1406,7 +1406,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
int d = r10_bio->devs[i].devnum;
if (r10_bio->devs[i].bio) {
struct md_rdev *rdev = conf->mirrors[d].rdev;
- mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ mbio = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
bio_trim(mbio, r10_bio->sector - bio->bi_iter.bi_sector,
max_sectors);
r10_bio->devs[i].bio = mbio;
@@ -1457,7 +1457,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
smp_mb();
rdev = conf->mirrors[d].rdev;
}
- mbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ mbio = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
bio_trim(mbio, r10_bio->sector - bio->bi_iter.bi_sector,
max_sectors);
r10_bio->devs[i].repl_bio = mbio;
@@ -2565,7 +2565,7 @@ static int narrow_write_error(struct r10bio *r10_bio, int i)
if (sectors > sect_to_write)
sectors = sect_to_write;
/* Write at 'sector' for 'sectors' */
- wbio = bio_clone_mddev(bio, GFP_NOIO, mddev);
+ wbio = bio_clone_fast_mddev(bio, GFP_NOIO, mddev);
bio_trim(wbio, sector - bio->bi_iter.bi_sector, sectors);
wsector = r10_bio->devs[i].addr + (sector - r10_bio->sector);
wbio->bi_iter.bi_sector = wsector +
@@ -2641,8 +2641,7 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
mdname(mddev),
bdevname(rdev->bdev, b),
(unsigned long long)r10_bio->sector);
- bio = bio_clone_mddev(r10_bio->master_bio,
- GFP_NOIO, mddev);
+ bio = bio_clone_fast_mddev(r10_bio->master_bio, GFP_NOIO, mddev);
bio_trim(bio, r10_bio->sector - bio->bi_iter.bi_sector, max_sectors);
r10_bio->devs[slot].bio = bio;
r10_bio->devs[slot].rdev = rdev;
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 34f76615d620..b0bf647dd414 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5056,9 +5056,9 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
return 0;
}
/*
- * use bio_clone_mddev to make a copy of the bio
+ * use bio_clone_fast_mddev to make a copy of the bio
*/
- align_bi = bio_clone_mddev(raid_bio, GFP_NOIO, mddev);
+ align_bi = bio_clone_fast_mddev(raid_bio, GFP_NOIO, mddev);
if (!align_bi)
return 0;
/*
--
2.7.4
^ permalink raw reply related
* [PATCH v1 4/5] md: remove unnecessary check on mddev
From: Ming Lei @ 2017-02-10 10:56 UTC (permalink / raw)
To: Shaohua Li, Jens Axboe, linux-kernel, linux-raid, linux-block,
Christoph Hellwig, NeilBrown
Cc: Ming Lei
In-Reply-To: <1486724177-14817-1-git-send-email-tom.leiming@gmail.com>
mddev is never NULL and neither is ->bio_set, so
remove the check.
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/md/md.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3425c2b779a6..2835f09b9e71 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -193,9 +193,6 @@ EXPORT_SYMBOL_GPL(bio_alloc_mddev);
struct bio *bio_clone_mddev(struct bio *bio, gfp_t gfp_mask,
struct mddev *mddev)
{
- if (!mddev || !mddev->bio_set)
- return bio_clone(bio, gfp_mask);
-
return bio_clone_bioset(bio, gfp_mask, mddev->bio_set);
}
EXPORT_SYMBOL_GPL(bio_clone_mddev);
--
2.7.4
^ permalink raw reply related
* RE: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: David Laight @ 2017-02-10 10:28 UTC (permalink / raw)
To: 'Johannes Thumshirn', Scott Bauer,
linux-nvme@lists.infradead.org
Cc: keith.busch@intel.com, arnd@arndb.de, hch@infradead.org,
linux-kernel@vger.kernel.org, axboe@fb.com,
linux-block@vger.kernel.org, jonathan.derrick@intel.com
In-Reply-To: <e82756b9-df77-6033-b6ce-fbf9aac3b9a8@suse.de>
From: Johannes Thumshirn
> Sent: 10 February 2017 07:46
> On 02/09/2017 06:20 PM, Scott Bauer wrote:
> > When CONFIG_KASAN is enabled, compilation fails:
Does CONFIG_KASAN allocate guard stack space around everything that
is passed by address?
That sounds completely brain-dead.
There are a lot of functions that have an 'int *' argument to return
a single value - and are never going to do anything else.
...
> > Moved all the ioctl structures off the stack and dynamically activate
> > using _IOC_SIZE()
...
>
> > - if (copy_from_user(&session, arg, sizeof(session)))
> > - return -EFAULT;
> > - return opal_erase_locking_range(dev, &session);
> > + ioctl_ptr = kzalloc(cmd_size, GFP_KERNEL);
> > + if (!ioctl_ptr)
> > + return -ENOMEM;
> > + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> > + ret = -EFAULT;
> > + goto out;
> > }
>
> Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
You either want the copy_from_user() or the memzero() not both.
ISTM there could be two 'library' functions, maybe:
void *get_ioctl_buf(unsigned int cmd, long arg)
to malloc the buffer, memzero/copy_from_user, returns -EFAULT if copy fails.
int put_ioctl_buf(int rval, unsigned int cmd, const void *buf)
does copy_to_user() if rval >= 0 and IOR_READ, then frees buf.
return value is rval unless the copyout fails.
David
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* [PATCHv5 2/2] loop: support 4k physical blocksize
From: Hannes Reinecke @ 2017-02-10 8:43 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Ming Lei, Martin Schwidefsky, linux-block,
Linux Kernel Mailinglist, Hannes Reinecke, Hannes Reinecke
In-Reply-To: <1486716188-38638-1-git-send-email-hare@suse.de>
When generating bootable VM images certain systems (most notably
s390x) require devices with 4k blocksize. This patch implements
a new flag 'LO_FLAGS_BLOCKSIZE' which will set the physical
blocksize to that of the underlying device, and allow to change
the logical blocksize for up to the physical blocksize.
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
drivers/block/loop.c | 36 +++++++++++++++++++++++++++++++-----
drivers/block/loop.h | 1 +
include/uapi/linux/loop.h | 3 +++
3 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8dae865..776b3c6 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -221,7 +221,8 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
}
static int
-figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
+figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit,
+ loff_t logical_blocksize)
{
loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
sector_t x = (sector_t)size;
@@ -233,6 +234,12 @@ static void __loop_update_dio(struct loop_device *lo, bool dio)
lo->lo_offset = offset;
if (lo->lo_sizelimit != sizelimit)
lo->lo_sizelimit = sizelimit;
+ if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) {
+ lo->lo_logical_blocksize = logical_blocksize;
+ blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
+ blk_queue_logical_block_size(lo->lo_queue,
+ lo->lo_logical_blocksize);
+ }
set_capacity(lo->lo_disk, x);
bd_set_size(bdev, (loff_t)get_capacity(bdev->bd_disk) << 9);
/* let user-space know about the new size */
@@ -814,6 +821,7 @@ static void loop_config_discard(struct loop_device *lo)
struct file *file = lo->lo_backing_file;
struct inode *inode = file->f_mapping->host;
struct request_queue *q = lo->lo_queue;
+ int lo_bits = blksize_bits(lo->lo_logical_blocksize);
/*
* We use punch hole to reclaim the free space used by the
@@ -833,7 +841,7 @@ static void loop_config_discard(struct loop_device *lo)
q->limits.discard_granularity = inode->i_sb->s_blocksize;
q->limits.discard_alignment = 0;
- blk_queue_max_discard_sectors(q, UINT_MAX >> 9);
+ blk_queue_max_discard_sectors(q, UINT_MAX >> lo_bits);
q->limits.discard_zeroes_data = 1;
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
}
@@ -922,6 +930,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
lo->use_dio = false;
lo->lo_blocksize = lo_blocksize;
+ lo->lo_logical_blocksize = 512;
lo->lo_device = bdev;
lo->lo_flags = lo_flags;
lo->lo_backing_file = file;
@@ -1087,6 +1096,7 @@ static int loop_clr_fd(struct loop_device *lo)
int err;
struct loop_func_table *xfer;
kuid_t uid = current_uid();
+ int lo_flags = lo->lo_flags;
if (lo->lo_encrypt_key_size &&
!uid_eq(lo->lo_key_owner, uid) &&
@@ -1116,9 +1126,24 @@ static int loop_clr_fd(struct loop_device *lo)
if (err)
return err;
+ if (info->lo_flags & LO_FLAGS_BLOCKSIZE) {
+ lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
+ if (LO_INFO_BLOCKSIZE(info) != 512 &&
+ LO_INFO_BLOCKSIZE(info) != 1024 &&
+ LO_INFO_BLOCKSIZE(info) != 2048 &&
+ LO_INFO_BLOCKSIZE(info) != 4096)
+ return -EINVAL;
+ if (LO_INFO_BLOCKSIZE(info) > lo->lo_blocksize)
+ return -EINVAL;
+ }
+
if (lo->lo_offset != info->lo_offset ||
- lo->lo_sizelimit != info->lo_sizelimit)
- if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit))
+ lo->lo_sizelimit != info->lo_sizelimit ||
+ lo->lo_flags != lo_flags ||
+ ((lo->lo_flags & LO_FLAGS_BLOCKSIZE) &&
+ lo->lo_logical_blocksize != LO_INFO_BLOCKSIZE(info)))
+ if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
+ info->lo_init[0]))
return -EFBIG;
loop_config_discard(lo);
@@ -1303,7 +1328,8 @@ static int loop_set_capacity(struct loop_device *lo)
if (unlikely(lo->lo_state != Lo_bound))
return -ENXIO;
- return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
+ return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit,
+ lo->lo_logical_blocksize);
}
static int loop_set_dio(struct loop_device *lo, unsigned long arg)
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index fb2237c..579f2f7 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -49,6 +49,7 @@ struct loop_device {
struct file * lo_backing_file;
struct block_device *lo_device;
unsigned lo_blocksize;
+ unsigned lo_logical_blocksize;
void *key_data;
gfp_t old_gfp_mask;
diff --git a/include/uapi/linux/loop.h b/include/uapi/linux/loop.h
index c8125ec..a3960f9 100644
--- a/include/uapi/linux/loop.h
+++ b/include/uapi/linux/loop.h
@@ -22,6 +22,7 @@ enum {
LO_FLAGS_AUTOCLEAR = 4,
LO_FLAGS_PARTSCAN = 8,
LO_FLAGS_DIRECT_IO = 16,
+ LO_FLAGS_BLOCKSIZE = 32,
};
#include <asm/posix_types.h> /* for __kernel_old_dev_t */
@@ -59,6 +60,8 @@ struct loop_info64 {
__u64 lo_init[2];
};
+#define LO_INFO_BLOCKSIZE(l) (l)->lo_init[0]
+
/*
* Loop filter types
*/
--
1.8.5.6
^ permalink raw reply related
* [PATCHv5 1/2] loop: Remove unused 'bdev' argument from loop_set_capacity
From: Hannes Reinecke @ 2017-02-10 8:43 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Ming Lei, Martin Schwidefsky, linux-block,
Linux Kernel Mailinglist, Hannes Reinecke
In-Reply-To: <1486716188-38638-1-git-send-email-hare@suse.de>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
---
drivers/block/loop.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f347285..8dae865 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1298,7 +1298,7 @@ static int loop_clr_fd(struct loop_device *lo)
return err;
}
-static int loop_set_capacity(struct loop_device *lo, struct block_device *bdev)
+static int loop_set_capacity(struct loop_device *lo)
{
if (unlikely(lo->lo_state != Lo_bound))
return -ENXIO;
@@ -1361,7 +1361,7 @@ static int lo_ioctl(struct block_device *bdev, fmode_t mode,
case LOOP_SET_CAPACITY:
err = -EPERM;
if ((mode & FMODE_WRITE) || capable(CAP_SYS_ADMIN))
- err = loop_set_capacity(lo, bdev);
+ err = loop_set_capacity(lo);
break;
case LOOP_SET_DIRECT_IO:
err = -EPERM;
--
1.8.5.6
^ permalink raw reply related
* [PATCHv5 0/2] loop: enable different logical blocksizes
From: Hannes Reinecke @ 2017-02-10 8:43 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Ming Lei, Martin Schwidefsky, linux-block,
Linux Kernel Mailinglist, Hannes Reinecke
Currently the loop driver just simulates 512-byte blocks. When
creating bootable images for virtual machines it might be required
to use a different physical blocksize (eg 4k for S/390 DASD), as
the some bootloaders (like lilo or zipl for S/390) need to know
the physical block addresses of the kernel and initrd.
With this patchset the loop driver will export the logical and
physical blocksize and the current LOOP_SET_STATUS64 ioctl is
extended to set the logical blocksize by re-using the existing
'init' fields, which are currently unused.
As usual, comments and reviews are welcome.
Changes to v1:
- Move LO_FLAGS_BLOCKSIZE definition
- Reshuffle patches
Changes to v2:
- Include reviews from Ming Lei
Changes to v3:
- Include reviews from Christoph
- Merge patches
Changes to v4:
- Add LO_INFO_BLOCKSIZE definition
Hannes Reinecke (2):
loop: Remove unused 'bdev' argument from loop_set_capacity
loop: support 4k physical blocksize
drivers/block/loop.c | 40 +++++++++++++++++++++++++++++++++-------
drivers/block/loop.h | 1 +
include/uapi/linux/loop.h | 3 +++
3 files changed, 37 insertions(+), 7 deletions(-)
--
1.8.5.6
^ permalink raw reply
* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Arnd Bergmann @ 2017-02-10 8:01 UTC (permalink / raw)
To: Scott Bauer
Cc: keith.busch, hch, linux-kernel, linux-nvme, axboe, David.Laight,
linux-block, jonathan.derrick
In-Reply-To: <1486660801-5105-3-git-send-email-scott.bauer@intel.com>
On Thursday, February 9, 2017 10:20:01 AM CET Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
>
> block/sed-opal.c: In function 'sed_ioctl':
> block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
>
> Moved all the ioctl structures off the stack and dynamically activate
> using _IOC_SIZE()
>
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Scott Bauer <scott.bauer@intel.com>
> ---
> block/sed-opal.c | 134 +++++++++++++++++++++----------------------------------
> 1 file changed, 50 insertions(+), 84 deletions(-)
>
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index bf1406e..4985d95 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -2346,7 +2346,10 @@ EXPORT_SYMBOL(opal_unlock_from_suspend);
>
> 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;
> + unsigned int cmd_size = _IOC_SIZE(cmd);
>
> if (!capable(CAP_SYS_ADMIN))
> return -EACCES;
We usually have a size check in there to avoid allocating large amounts
of memory. _IOC_SIZEBITS is 14, so you can have up to 16kb here, which
is probably ok, but I'd recommend either adding a comment to say that
it is, or just checking against the largest realistic size.
Having something like v4l with their tables if ioctl commands and
function pointers would also solve it, as you'd be checking for valid
command numbers before doing the copy then.
Otherwise looks good to me.
Arnd
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH V3 2/2] Move stack parameters for sed_ioctl to prevent oversized stack with CONFIG_KASAN
From: Johannes Thumshirn @ 2017-02-10 7:45 UTC (permalink / raw)
To: Scott Bauer, linux-nvme
Cc: keith.busch, arnd, hch, linux-kernel, axboe, David.Laight,
linux-block, jonathan.derrick
In-Reply-To: <1486660801-5105-3-git-send-email-scott.bauer@intel.com>
On 02/09/2017 06:20 PM, Scott Bauer wrote:
> When CONFIG_KASAN is enabled, compilation fails:
> =
> block/sed-opal.c: In function 'sed_ioctl':
> block/sed-opal.c:2447:1: error: the frame size of 2256 bytes is larger th=
an 2048 bytes [-Werror=3Dframe-larger-than=3D]
> =
> Moved all the ioctl structures off the stack and dynamically activate
> using _IOC_SIZE()
> =
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> =
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Scott Bauer <scott.bauer@intel.com>
> ---
> block/sed-opal.c | 134 +++++++++++++++++++++----------------------------=
------
> 1 file changed, 50 insertions(+), 84 deletions(-)
> =
[...]
> - if (copy_from_user(&session, arg, sizeof(session)))
> - return -EFAULT;
> - return opal_erase_locking_range(dev, &session);
> + ioctl_ptr =3D kzalloc(cmd_size, GFP_KERNEL);
> + if (!ioctl_ptr)
> + return -ENOMEM;
> + if (copy_from_user(ioctl_ptr, arg, cmd_size)) {
> + ret =3D -EFAULT;
> + goto out;
> }
Can't we use memdup_user() instead of kzalloc() + copy_from_user()?
-- =
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N=FCrnberg
GF: Felix Imend=F6rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N=FCrnberg)
Key fingerprint =3D EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [lkp-robot] [scsi, block] 0dba1314d4: WARNING:at_fs/sysfs/dir.c:#sysfs_warn_dup
From: Dan Williams @ 2017-02-10 3:11 UTC (permalink / raw)
To: James Bottomley
Cc: Jens Axboe, 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: <1486598898.2484.46.camel@HansenPartnership.com>
[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]
On Wed, Feb 8, 2017 at 4:08 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2017-02-06 at 21:42 -0800, Dan Williams wrote:
[..]
>> ...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"
I gave this a shot on top of linux-next, but still hit the failure.
Log attached.
[-- Attachment #2: log --]
[-- Type: application/octet-stream, Size: 8819 bytes --]
[ 5.980959] st: Version 20160209, fixed bufsize 32768, s/g segs 256
[ 5.991170] osst :I: Tape driver with OnStream support version 0.99.4
[ 5.991170] osst :I: $Id: osst.c,v 1.73 2005/01/01 21:13:34 wriede Exp $
[ 5.993376] SCSI Media Changer driver v0.25
[ 5.995157] scsi host0: scsi_debug: version 1.86 [20160430]
[ 5.995157] dev_size_mb=8, opts=0x0, submit_queues=1, statistics=0
[ 5.997479] scsi 0:0:0:0: Direct-Access Linux scsi_debug 0186 PQ: 0 ANSI: 7
[ 6.030073] sd 0:0:0:0: [sda] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
[ 6.050047] sd 0:0:0:0: [sda] Write Protect is off
[ 6.051006] sd 0:0:0:0: [sda] Mode Sense: 73 00 10 08
[ 6.090067] sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, supports DPO and FUA
[ 6.510044] sd 0:0:0:0: [sda] Attached SCSI disk
[ 6.511614] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[ 6.512740] sd 0:0:0:0: Attached scsi generic sg0 type 0
[ 6.550065] sd 0:0:0:0: [sdb] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB)
[ 6.570050] sd 0:0:0:0: [sdb] Write Protect is off
[ 6.570996] sd 0:0:0:0: [sdb] Mode Sense: 73 00 10 08
[ 6.610113] sd 0:0:0:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
[ 6.670309] kobject (d5cb36a4): tried to init an initialized object, something is seriously wrong.
[ 6.673969] CPU: 0 PID: 100 Comm: kworker/u2:1 Not tainted 4.10.0-rc7-next-20170207-00005-g43256e7301aa #950
[ 6.678132] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.1-1.fc24 04/01/2014
[ 6.680063] Workqueue: events_unbound async_run_entry_fn
[ 6.680063] Call Trace:
[ 6.680063] dump_stack+0x79/0xa4
[ 6.680063] kobject_init+0x75/0x90
[ 6.680063] ? blk_mq_disable_hotplug+0xf/0x20
[ 6.680063] blk_mq_register_dev+0x2a/0x120
[ 6.680063] blk_register_queue+0x7f/0x160
[ 6.680063] device_add_disk+0x1e3/0x480
[ 6.680063] sd_probe_async+0xf1/0x1c0
[ 6.680063] ? __lock_acquire.isra.14+0x43b/0x940
[ 6.680063] async_run_entry_fn+0x30/0x190
[ 6.680063] ? process_one_work+0x12f/0x430
[ 6.680063] process_one_work+0x1aa/0x430
[ 6.680063] ? process_one_work+0x12f/0x430
[ 6.680063] worker_thread+0x1dd/0x470
[ 6.680063] kthread+0xd4/0x100
[ 6.680063] ? process_one_work+0x430/0x430
[ 6.680063] ? __kthread_create_on_node+0x180/0x180
[ 6.680063] ret_from_fork+0x21/0x2c
[ 6.706694] kobject (ffab2dec): tried to init an initialized object, something is seriously wrong.
[ 6.709061] CPU: 0 PID: 100 Comm: kworker/u2:1 Not tainted 4.10.0-rc7-next-20170207-00005-g43256e7301aa #950
[ 6.711577] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.1-1.fc24 04/01/2014
[ 6.713896] Workqueue: events_unbound async_run_entry_fn
[ 6.715240] Call Trace:
[ 6.716160] dump_stack+0x79/0xa4
[ 6.716677] kobject_init+0x75/0x90
[ 6.716677] blk_mq_register_dev+0x4c/0x120
[ 6.716677] blk_register_queue+0x7f/0x160
[ 6.716677] device_add_disk+0x1e3/0x480
[ 6.716677] sd_probe_async+0xf1/0x1c0
[ 6.716677] ? __lock_acquire.isra.14+0x43b/0x940
[ 6.716677] async_run_entry_fn+0x30/0x190
[ 6.716677] ? process_one_work+0x12f/0x430
[ 6.716677] process_one_work+0x1aa/0x430
[ 6.716677] ? process_one_work+0x12f/0x430
[ 6.716677] worker_thread+0x1dd/0x470
[ 6.716677] kthread+0xd4/0x100
[ 6.716677] ? process_one_work+0x430/0x430
[ 6.716677] ? __kthread_create_on_node+0x180/0x180
[ 6.716677] ret_from_fork+0x21/0x2c
[ 6.734034] ------------[ cut here ]------------
[ 6.735233] WARNING: CPU: 0 PID: 100 at ./include/linux/kref.h:54 kobject_get+0x7f/0x90
[ 6.737285] CPU: 0 PID: 100 Comm: kworker/u2:1 Not tainted 4.10.0-rc7-next-20170207-00005-g43256e7301aa #950
[ 6.739579] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.1-1.fc24 04/01/2014
[ 6.741722] Workqueue: events_unbound async_run_entry_fn
[ 6.742966] Call Trace:
[ 6.743809] dump_stack+0x79/0xa4
[ 6.743899] __warn+0xd2/0xf0
[ 6.743899] ? kobject_get+0x7f/0x90
[ 6.743899] warn_slowpath_null+0x25/0x30
[ 6.743899] kobject_get+0x7f/0x90
[ 6.743899] kobject_add_internal+0x2e/0x360
[ 6.743899] ? kfree_const+0x18/0x20
[ 6.743899] ? kobject_set_name_vargs+0x62/0x80
[ 6.743899] kobject_add+0x35/0x80
[ 6.743899] ? up_write+0x16/0x40
[ 6.743899] blk_mq_register_hctx+0x95/0xc0
[ 6.743899] blk_mq_register_dev+0xd9/0x120
[ 6.743899] blk_register_queue+0x7f/0x160
[ 6.743899] device_add_disk+0x1e3/0x480
[ 6.743899] sd_probe_async+0xf1/0x1c0
[ 6.743899] ? __lock_acquire.isra.14+0x43b/0x940
[ 6.743899] async_run_entry_fn+0x30/0x190
[ 6.743899] ? process_one_work+0x12f/0x430
[ 6.743899] process_one_work+0x1aa/0x430
[ 6.743899] ? process_one_work+0x12f/0x430
[ 6.743899] worker_thread+0x1dd/0x470
[ 6.743899] kthread+0xd4/0x100
[ 6.743899] ? process_one_work+0x430/0x430
[ 6.743899] ? __kthread_create_on_node+0x180/0x180
[ 6.743899] ret_from_fork+0x21/0x2c
[ 6.768405] ---[ end trace 7b77f5d77668026b ]---
[ 6.920058] sd 0:0:0:0: [sdb] Attached SCSI disk
[ 6.921534] sd 0:0:0:0: [sdb] Synchronizing SCSI cache
[ 6.950611] scsi host0: scsi_debug: version 1.86 [20160430]
[ 6.950611] dev_size_mb=8, opts=0x0, submit_queues=1, statistics=0
[ 6.953078] scsi 0:0:0:0: Direct-Access Linux scsi_debug 0186 PQ: 0 ANSI: 7
[ 6.955028] ------------[ cut here ]------------
[ 6.956026] WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x58/0x70
[ 6.957731] sysfs: cannot create duplicate filename '/class/scsi_disk/0:0:0:0'
[ 6.959408] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.10.0-rc7-next-20170207-00005-g43256e7301aa #950
[ 6.960027] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.1-1.fc24 04/01/2014
[ 6.960027] Call Trace:
[ 6.960027] dump_stack+0x79/0xa4
[ 6.960027] __warn+0xd2/0xf0
[ 6.960027] ? sysfs_warn_dup+0x58/0x70
[ 6.960027] warn_slowpath_fmt+0x36/0x40
[ 6.960027] sysfs_warn_dup+0x58/0x70
[ 6.960027] sysfs_do_create_link_sd.isra.0+0x9e/0xb0
[ 6.960027] sysfs_create_link+0x20/0x40
[ 6.960027] device_add+0x218/0x610
[ 6.960027] ? kvasprintf_const+0x49/0x60
[ 6.960027] ? kobject_set_name_vargs+0x62/0x80
[ 6.960027] sd_probe+0x31a/0x390
[ 6.960027] ? _raw_spin_unlock+0x1d/0x30
[ 6.960027] driver_probe_device+0x190/0x4a0
[ 6.960027] __device_attach_driver+0x6f/0x100
[ 6.960027] ? klist_next+0x6e/0x100
[ 6.960027] ? __driver_attach+0xf0/0xf0
[ 6.960027] bus_for_each_drv+0x47/0x80
[ 6.960027] __device_attach+0xa8/0x120
[ 6.960027] ? __driver_attach+0xf0/0xf0
[ 6.960027] device_initial_probe+0xd/0x10
[ 6.960027] bus_probe_device+0x77/0x80
[ 6.960027] device_add+0x320/0x610
[ 6.960027] scsi_sysfs_add_sdev+0x85/0x2b0
[ 6.960027] ? scsi_attach_vpd+0x1f9/0x210
[ 6.960027] scsi_probe_and_add_lun+0xd44/0xe70
[ 6.960027] __scsi_scan_target+0xd8/0x690
[ 6.960027] ? __pm_runtime_resume+0x37/0x90
[ 6.960027] scsi_scan_channel+0x8f/0xb0
[ 6.960027] scsi_scan_host_selected+0x100/0x180
[ 6.960027] do_scsi_scan_host+0x8a/0x90
[ 6.960027] scsi_scan_host+0x15a/0x1a0
[ 6.960027] sdebug_driver_probe+0x14f/0x3d0
[ 6.960027] ? _raw_spin_unlock+0x1d/0x30
[ 6.960027] ? devices_kset_move_last+0x71/0xc0
[ 6.960027] ? sysfs_create_link+0x20/0x40
[ 6.960027] driver_probe_device+0xd4/0x4a0
[ 6.960027] __device_attach_driver+0x6f/0x100
[ 6.960027] ? klist_next+0x6e/0x100
[ 6.960027] ? __driver_attach+0xf0/0xf0
[ 6.960027] bus_for_each_drv+0x47/0x80
[ 6.960027] __device_attach+0xa8/0x120
[ 6.960027] ? __driver_attach+0xf0/0xf0
[ 6.960027] device_initial_probe+0xd/0x10
[ 6.960027] bus_probe_device+0x77/0x80
[ 6.960027] device_add+0x320/0x610
[ 6.960027] ? pm_runtime_init+0xea/0xf0
[ 6.960027] device_register+0x12/0x20
[ 6.960027] sdebug_add_adapter+0xda/0x1e0
[ 6.960027] ? driver_register+0x83/0xe0
[ 6.960027] scsi_debug_init+0x5a0/0x6eb
[ 6.960027] ? kobject_uevent+0xa/0x10
[ 6.960027] ? driver_register+0x83/0xe0
[ 6.960027] ? scsi_register_driver+0xf/0x20
[ 6.960027] ? init_ch_module+0x9d/0x9d
[ 6.960027] do_one_initcall+0x7b/0x132
[ 6.960027] ? kernel_init_freeable+0xe7/0x183
[ 6.960027] kernel_init_freeable+0x10a/0x183
[ 6.960027] ? rest_init+0xb0/0xb0
[ 6.960027] kernel_init+0xb/0x100
[ 6.960027] ? schedule_tail+0xc/0x70
[ 6.960027] ? rest_init+0xb0/0xb0
[ 6.960027] ret_from_fork+0x21/0x2c
[ 7.030980] ---[ end trace 7b77f5d77668026c ]---
[ 7.032025] sd: probe of 0:0:0:0 failed with error -17
[ 7.033172] scsi 0:0:0:0: Attached scsi generic sg0 type 0
^ permalink raw reply
* Re: [PATCH 04/10] block: Move bdi_unregister() to del_gendisk()
From: NeilBrown @ 2017-02-10 2:21 UTC (permalink / raw)
To: Jan Kara, Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-5-jack@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 1475 bytes --]
On Thu, Feb 09 2017, 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().
What an amazingly sensible idea!! I like that a lot.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 07/10] writeback: Implement reliable switching to default writeback structure
From: NeilBrown @ 2017-02-10 2:19 UTC (permalink / raw)
To: Jan Kara, Jens Axboe
Cc: linux-block, Christoph Hellwig, Tejun Heo, Dan Williams,
Thiago Jung Bauermann, NeilBrown, Jan Kara
In-Reply-To: <20170209124433.2626-8-jack@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 7516 bytes --]
On Thu, Feb 09 2017, Jan Kara wrote:
> 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.
I think you mean "set_current_state()" ??
It's rather a trap for the unwary, this need for a smp_mb().
Greping for wake_up_bit(), I find quite a few places with barriers -
sometimes clear_bit_unlock() or spin_unlock() - but
fs/block_dev.c- whole->bd_claiming = NULL;
fs/block_dev.c: wake_up_bit(&whole->bd_claiming, 0);
fs/cifs/connect.c- clear_bit(TCON_LINK_PENDING, &tlink->tl_flags);
fs/cifs/connect.c: wake_up_bit(&tlink->tl_flags, TCON_LINK_PENDING);
fs/cifs/misc.c- clear_bit(CIFS_INODE_PENDING_WRITERS, &cinode->flags);
fs/cifs/misc.c: wake_up_bit(&cinode->flags, CIFS_INODE_PENDING_WRITERS);
(several more in cifs)
net/sunrpc/xprt.c- clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
net/sunrpc/xprt.c- xprt->ops->close(xprt);
net/sunrpc/xprt.c- xprt_release_write(xprt, NULL);
net/sunrpc/xprt.c: wake_up_bit(&xprt->state, XPRT_LOCKED);
(there might be a barrier in ->close or xprt_release_write() I guess)
security/keys/gc.c- clear_bit(KEY_GC_REAPING_KEYTYPE, &key_gc_flags);
security/keys/gc.c: wake_up_bit(&key_gc_flags, KEY_GC_REAPING_KEYTYPE);
I wonder if there is a good way to make this less error-prone.
I would suggest that wake_up_bit() should always have a barrier, and
__wake_up_bit() is needed to avoid it, but there is already a
__wake_up_bit() with a slightly different interface.
In this case, you have a spin_unlock() just before the wake_up_bit().
It is my understand that it would provide enough of a barrier (all
writes before are globally visible after), so do you really need
the barrier here?
> + */
> + 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);
I note that __inode_wait_for_state_bit() can drop and reclaim ->i_lock.
is it possible that:
!inode_to_wb_is_valid(inode) || inode_to_wb(inode) == &bdi->wb)
could change while ->i_lock is unlocked?
It would be particular unfortunate if inode_to_wb(inode) became &bdi->wb
due to some thing thread, as do_inode_switch_wbs() will deadlock if
inode_to_wb(inode) == &bdi->wb
i.e. do you need to repeat the test?
Thanks,
NeilBrown
> + 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] dm-zoned: Zoned block device target
From: Damien Le Moal @ 2017-02-10 1:24 UTC (permalink / raw)
To: Hannes Reinecke, dm-devel, Alasdair Kergon, Mike Snitzer
Cc: Christoph Hellwig, linux-block
In-Reply-To: <d2353e66-183c-1f71-7a64-52e4b862bc17@suse.de>
Hannes,
On 2/10/17 01:07, Hannes Reinecke wrote:
>> +/*
>> + * CRC32
>> + */
>> +static u32 dmz_sb_crc32(u32 crc, const void *buf, size_t length)
>> +{
>> + unsigned char *p = (unsigned char *)buf;
>> + int i;
>> +
>> +#define CRCPOLY_LE 0xedb88320
>> +
>> + while (length--) {
>> + crc ^= *p++;
>> + for (i = 0; i < 8; i++)
>> + crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY_LE : 0);
>> + }
>> +
>> + return crc;
>> +}
>> +
> Why do you insist on your own CRC32 implementations?
> Don't we have enough already?
Indeed. I will change this to use existing kernel API.
> Have you looked at using dm-kcopyd for doing the reclaim?
> What's inhibiting you from using it?
I did look at it, and I did like it very much. It would be much more
simple to use than open coding everything like I did. But the problem is
that while the read part is fine, the write part is not. The reason is
that most of the time, the zone being reclaimed is badly fragmented
(valid blocks all over the place in the zone). Reading those valid
blocks with kcopyd is fine, but when moving them to the target zone, the
overall write sequence MUST be sequential, which means that
blkdev_issue_zeroout calls must be inserted between non sequential
regions (the indirection system is per zone, not per block, so the
relative offsets of the blocks read in the zone being reclaimed must be
preserved in the destination zone).
I could modify kcopyd_copy to do this zeroout insertion of course. In
fact, I think that would be the better approach. But I think that can be
done later, once/if Mike accepts dm-zoned. Or I could re-submit dm-zoned
together with a patch for kcopyd_copy.
Mike,
Any advice on which approach is best/more acceptable for you ?
Thanks !
Best regards.
--
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com
^ permalink raw reply
* Re: [PATCH 1/1] dm-zoned: Zoned block device target
From: Damien Le Moal @ 2017-02-10 0:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: dm-devel, Alasdair Kergon, Mike Snitzer, Hannes Reinecke,
linux-block
In-Reply-To: <20170209093606.GA8189@lst.de>
Christoph,
On 2/9/17 18:36, Christoph Hellwig wrote:
> 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.
Thank you for the hint. I will look into this.
Best regards.
--
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@wdc.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com
^ 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