Re: [PATCH 11/20] block: reference struct block_device from struct hd_struct

[Jan Kara <jack@xxxxxxx>]

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