On Tue, Nov 17, 2015 at 9:56 PM, Tejun Heo <tj@xxxxxxxxxx> wrote:
> Hello, Ilya.
>
> On Mon, Nov 16, 2015 at 09:59:18PM +0100, Ilya Dryomov wrote:
> ...
>> Looking at __blkdev_put(), the issue becomes clear: we are taking
>> precautions to flush before calling out to ->release() because, at
>> least according to the comment, ->release() can free queue; we are
>> recording owner pointer because put_disk() may free both gendisk and
>> queue, and then, after all that, we are calling bdput() which may
>> touch the queue through wb_put() in inode_detach_wb(). (The fun part
>> is wb_put() is supposed to be a noop for root wbs, but slab debugging
>> interferes with that by poisoning wb->bdi pointer.)
>>
>> 1514 * dirty data before.
>> 1515 */
>> 1516 bdev_write_inode(bdev);
>> 1517 }
>> 1518 if (bdev->bd_contains == bdev) {
>> 1519 if (disk->fops->release)
>> 1520 disk->fops->release(disk, mode);
>> 1521 }
>> 1522 if (!bdev->bd_openers) {
>> 1523 struct module *owner = disk->fops->owner;
>> 1524
>> 1525 disk_put_part(bdev->bd_part);
>> 1526 bdev->bd_part = NULL;
>> 1527 bdev->bd_disk = NULL;
>> 1528 if (bdev != bdev->bd_contains)
>> 1529 victim = bdev->bd_contains;
>> 1530 bdev->bd_contains = NULL;
>> 1531
>> 1532 put_disk(disk); <-- may free q
>> 1533 module_put(owner);
>> 1534 }
>> 1535 mutex_unlock(&bdev->bd_mutex);
>> 1536 bdput(bdev); <-- may touch q.backing_dev_info.wb
>
> Ah, that's a sneaky bug. Thanks a lot for chasing it down. The
> scenario sounds completely plausible to me.
>
>> To reproduce, apply the attached patch (systemd-udevd condition is just
>> a convenience: udev reacts to change events by getting the bdev which
>> it then has to put), boot with slub_debug=,blkdev_queue and do:
>>
>> $ sudo modprobe loop
>> $ sudo losetup /dev/loop0 foo.img
>> $ sudo dd if=/dev/urandom of=/dev/loop0 bs=1M count=1
>> $ sudo losetup -d /dev/loop0
>> $ sudo rmmod loop
>>
>> (rmmod is key - it's the only way to get loop to do put_disk(). For
>> rbd, it's just rbd map - dd - rbd unmap.)
>>
>> In the past we used to reassign to default_backing_dev_info here, but
>> it was nuked in b83ae6d42143 ("fs: remove mapping->backing_dev_info").
>
> Woohoo, it wasn't me. :)
Well, you and Christoph have been pulling it in different directions.
He removed default_backing_dev_info along with mapping->backing_dev_info,
but you then kind of readded this link with inode->i_wb.
>
>> Shortly after that cgroup-specific writebacks patches from Tejun got
>> merged, adding inode::i_wb and inode_detach_wb() call. The fix seems
>> to be to detach the inode earlier, but I'm not familiar enough with
>> cgroups code, so sending my findings instead of a patch. Christoph,
>> Tejun?
>
> It's stinky that the bdi is going away while the inode is still there.
> Yeah, blkdev inodes are special and created early but I think it makes
> sense to keep the underlying structures (queue and bdi) around while
> bdev is associated with it. Would simply moving put_disk() after
> bdput() work?
I'd think so. struct block_device is essentially a "block device"
pseudo-filesystem inode, and as such, may not be around during the
entire lifetime of gendisk / queue. It may be kicked out of the inode
cache as soon as the device is closed, so it makes sense to put it
before putting gendisk / queue, which will outlive it.
However, I'm confused by this comment
/*
* ->release can cause the queue to disappear, so flush all
* dirty data before.
*/
bdev_write_inode(bdev);
It's not true, at least since your 523e1d399ce0 ("block: make gendisk
hold a reference to its queue"), right? (It used to say "->release can
cause the old bdi to disappear, so must switch it out first" and was
changed by Christoph in the middle of his backing_dev_info series.)
Thanks,
Ilya
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
