On Tue, 14 Jan 2014 18:14:46 +0100 Nicolas Schichan <nschic...@freebox.fr> wrote:
>
>
> Hi,
>
> I have recently been trying to find the cause a livelock occurring during MD
> device open.
>
> The livelock happens when a process tries to open an MD device for the
> first time and another opens the same MD device and sends an invalid
> ioctl:
>
> Process 1 Process 2
> --------- ---------
>
> md_alloc()
> mddev_find()
> -> returns a new mddev with
> hold_active == UNTIL_IOCTL
> add_disk()
> -> sends KOBJ_ADD uevent
>
> (sees KOBJ_ADD uevent for device)
> md_open()
> md_ioctl(INVALID_IOCTL)
> -> returns ENODEV and clears
> mddev->hold_active
> md_release()
> md_put()
> -> deletes the mddev as
> hold_active is 0
>
> md_open()
> mddev_find()
> -> returns a newly
> allocated mddev with
> mddev->gendisk == NULL
> -> returns with ERESTARTSYS
> (kernel restarts the open syscall)
>
>
> As to how to fix this, I see two possibilities:
>
> - don't set hold_active to 0 if err is -ENODEV in the abort_unlock
> path in md_ioctl().
>
> - check cmd parameter early in md_ioctl() and return -ENOTTY if the
> cmd parameter is not a valid MD ioctl.
>
> Please advise on the preferred way to fix this, I'll be glad to send a
> patch for whatever is the preferred solution.
>
> I have also a simple C program that I can send should you want to reproduce
> the issue.
>
> Regards,
>
That's a very small race you are consistently losing - if I understand
correctly.
In __blkdev_get:
restart:
ret = -ENXIO;
disk = get_gendisk(bdev->bd_dev, &partno);
if (!disk)
goto out;
owner = disk->fops->owner;
disk_block_events(disk);
mutex_lock_nested(&bdev->bd_mutex, for_part);
The "get_gendisk" calls into md_alloc (via md_probe) and then add_disk(),
which generates a uevent which is handled by udev.
And before the above code gets to the mutex_lock_nexted(), the process run by
udev must have opened the device (executing all that code above and more) and
issued the ioctl.
I guess it is possible, but happening every time to produce a live-lock
suggests that the scheduler must be encouraging that behaviour. Presumably
this is a virtual machine with just one CPU ??
I suppose the best fix is, as you suggest, to avoid clearing hold_active for
invalid ioctls. It feels a bit like papering over a bug, but I think the
only way to really fix it is to add extra locking to the above code sequence
and I don't want to do that.
Of your two suggestions I much prefer the second. It will be more code, but
it is also more obviously correct. The current code is rather messy with
respect to invalid ioctl commands.
I would be happy to accept a patch which aborted md_ioctl if the cmd wasn't
one of those known to md.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
