Re: [patch 1/2] raid6_end_write_request() spinlock fix

On Tue, Apr 25, 2006 at 04:50:10PM +1000, Neil Brown wrote:
On Tuesday April 25, qiyong@xxxxxxxxx wrote:
On Tue, Apr 25, 2006 at 03:13:49PM +1000, Neil Brown wrote:
On Tuesday April 25, qiyong@xxxxxxxxx wrote:
Hello,

Reduce the raid6_end_write_request() spinlock window.

Andrew: please don't include these in -mm. This one and the
corresponding raid5 are wrong, and I'm not sure yet the unplug_device
changes.

I am sure with the unplug_device. Just look follow the path...


What path? There are probably several. If I follow the path, will I
see the same things as you see? Who knows, because you haven't
bothered to tell us what you see.

There are only two places where handle_list is possibly re-filled:
__release_stripe() and raidX_activate_delayed(). So raidXd should only
wakeup after these two points.



Yes. Let's fix the error(). In any case, the current code is broken. (see raid5/6_end_read_request)

What will I see in raidX_end_read_request. Surely it isn't that hard
to write a few more sentences?

You should see md_error() in raidX_end_read_request isn't in any spinlocks.

conf->working_disks isn't atomic_t and so decrementing without a
spinlock isn't safe. So lets just leave it all inside a spinlock.

test_and_set_bit(Faulty, &rdev->flags) protects it as well imho.
It can be enter only once.


Also I have a vague memory that clearing In_sync before Faulty is
important, but I'm not certain of that.

Maybe, but seems not apply here.


Remember: the code is there for a reason. It might not be a good
reason, and the code could well be wrong. But it would be worth your
effort trying to find out what the reason is before blithely changing
it (as I discovered recently with a change I suggested to
invalidate_mapping_pages).

Thanks :)
--
Coywolf Qi Hunt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [BUG] long freezes on thinkpad t60
    ... On Thu, 21 Jun 2007, Jarek Poplawski wrote: ... One of them is this loop, ... course we can't see any reason for this, ... MUST NOT release and immediately re-acquire the same spinlock on the same ...
    (Linux-Kernel)
  • Re: DIRQL vs DISPATCH (Was: Re: About the Problem with the DDKs PortIO SAMPLE.)
    ... Of course the reason is to avoid having most spinlocks be required to preempt all interrupts. ... The scheduler is run at DISPATCH_LEVEL, so anything that might page fault or otherwise block on thread scheduling clearly can't be run at DISPATCH_LEVEL. ... However, the scheduler doesn't run at DIRQL either of course, and it seems like it should be equally limited in most cases. ... spinlock around this is not-so-good idea. ...
    (microsoft.public.development.device.drivers)
  • Re: Using RCU with rcu_read_lock()?
    ... >>> care about object owning the spinlock and so acquiring the spinlock is ... >> Yes, preemptible rcu requires that you use the full interface, also, it ... >> Please do use the RCU interface in full. ... other reason anyway. ...
    (Linux-Kernel)
  • Re: [patch 1/2] raid6_end_write_request() spinlock fix
    ... Reduce the raid6_end_write_requestspinlock window. ... Andrew: please don't include these in -mm. ... corresponding raid5 are wrong, and I'm not sure yet the unplug_device ... the code is there for a reason. ...
    (Linux-Kernel)