On Wed 18-11-20 09:47:51, Christoph Hellwig wrote:
> To simplify block device lookup and a few other upcomdin areas, make sure
> that we always have a struct block_device available for each disk and
> each partition. The only downside of this is that each device and
> partition uses a little more memories. The upside will be that a lot of
^^^ memory
> code can be simplified.
>
> With that all we need to look up the block device is to lookup the inode
> and do a few sanity checks on the gendisk, instead of the separate lookup
> for the gendisk.
>
> As part of the change switch bdget() to only find existing block devices,
> given that we know that the block_device structure must be allocated at
> probe / partition scan time.
>
> blk-cgroup needed a bit of a special treatment as the only place that
> wanted to lookup a gendisk outside of the normal blkdev_get path. It is
> switched to lookup using the block device hash now that this is the
> primary lookup path.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
The patch looks good to me and I like the simplifications! I've found just
one small issue below.
> @@ -1748,16 +1600,18 @@ struct gendisk *__alloc_disk_node(int minors, int node_id)
> if (!disk)
> return NULL;
>
> + disk->part0.bdev = bdev_alloc(disk, 0);
> + if (!disk->part0.bdev)
> + goto out_free_disk;
> +
> disk->part0.dkstats = alloc_percpu(struct disk_stats);
> if (!disk->part0.dkstats)
> - goto out_free_disk;
> + goto out_bdput;
>
> init_rwsem(&disk->lookup_sem);
> disk->node_id = node_id;
> - if (disk_expand_part_tbl(disk, 0)) {
> - free_percpu(disk->part0.dkstats);
> - goto out_free_disk;
> - }
> + if (disk_expand_part_tbl(disk, 0))
> + goto out_free_bdstats;
>
> ptbl = rcu_dereference_protected(disk->part_tbl, 1);
> rcu_assign_pointer(ptbl->part[0], &disk->part0);
> @@ -1772,8 +1626,10 @@ struct gendisk *__alloc_disk_node(int minors, int node_id)
> * converted to make use of bd_mutex and sequence counters.
> */
> hd_sects_seq_init(&disk->part0);
> - if (hd_ref_init(&disk->part0))
> - goto out_free_part0;
> + if (hd_ref_init(&disk->part0)) {
> + hd_free_part(&disk->part0);
Aren't you missing kfree(disk) here?
> + return NULL;
> + }
>
> disk->minors = minors;
> rand_initialize_disk(disk);
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
