On Tue, 14 Jan 2014 18:14:46 +0100 Nicolas Schichan <nschichan@xxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
