Re: [PATCH] pstore/blk: Use the normal block device I/O path

[Christoph Hellwig]

> -	if (!dev || !dev->total_size || !dev->read || !dev->write)
> +	if (!dev || !dev->total_size || !dev->read || !dev->write) {
> +		if (!dev)
> +			pr_err("NULL device info\n");
> +		else {
> +			if (!dev->total_size)
> +				pr_err("zero sized device\n");
> +			if (!dev->read)
> +				pr_err("no read handler for device\n");
> +			if (!dev->write)
> +				pr_err("no write handler for device\n");
> +		}
>  		return -EINVAL;
> +	}

This is completely unrelated and should be a separate patch.  And it
also looks rather strange, I'd at very least split the dev check out
and return early without the weird compound statement, but would probably
handle each one separate.  All assuming that we really need all these
debug printks.

>  /*
>   * This takes its configuration only from the module parameters now.
>   */
>  static int __register_pstore_blk(void)

This needs a __init annotation now.

>
>
>  {
> +	struct pstore_device_info dev = {
> +		.read = psblk_generic_blk_read,
> +		.write = psblk_generic_blk_write,
> +	};

On-stack method tables are a little odd..

> +	if (!__is_defined(MODULE)) {

This looks a little weird.  Can we define a rapper for this in config.h
that is a little more self-explanatory, e.g. in_module()?

> +	if (!psblk_file->f_mapping)
> +		pr_err("missing f_mapping\n");

Can't ever be true.

> +	else if (!psblk_file->f_mapping->host)
> +		pr_err("missing host\n");

Can't ever be true either.

> +	else if (!I_BDEV(psblk_file->f_mapping->host))
> +		pr_err("missing I_BDEV\n");
> +	else if (!I_BDEV(psblk_file->f_mapping->host)->bd_inode)
> +		pr_err("missing bd_inode\n");

І_BDEV just does pointer arithmetics, so it can't ever return NULL.
And there are no block device inodes without bd_inode either.  And
all of this is per definition present for open S_ISBLK inodes.