Re: [PATCH] pstore: Refactor compression initialization

[Sai Prakash Ranjan]

On 10/18/2018 7:00 AM, Joel Fernandes wrote:
On Wed, Oct 17, 2018 at 02:41:24PM -0700, Kees Cook wrote:
> > With compression initialization now separated from pstore_register(),
> > there is no longer a good reason to do compression method selection
> > during fs init. Instead, merge everything together into the late init.
> > Additionally cleans up the reporting to be more clear.
> >
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> > This patch goes on top of Joel's. I'll have both in linux-next shortly...
> > ---
> >   fs/pstore/inode.c    |  2 --
> >   fs/pstore/internal.h |  3 --
> >   fs/pstore/platform.c | 65 ++++++++++++++++++++++++++++----------------
> >   3 files changed, 41 insertions(+), 29 deletions(-)
> >
> > diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> > index 5fcb845b9fec..d814723fb27d 100644
> > --- a/fs/pstore/inode.c
> > +++ b/fs/pstore/inode.c
> > @@ -486,8 +486,6 @@ static int __init init_pstore_fs(void)
> >   {
> >   	int err;
> >   
> > -	pstore_choose_compression();
> > -
> >   	/* Create a convenient mount point for people to access pstore */
> >   	err = sysfs_create_mount_point(fs_kobj, "pstore");
> >   	if (err)
> > diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> > index fb767e28aeb2..c029314478fa 100644
> > --- a/fs/pstore/internal.h
> > +++ b/fs/pstore/internal.h
> > @@ -37,7 +37,4 @@ extern bool	pstore_is_mounted(void);
> >   extern void	pstore_record_init(struct pstore_record *record,
> >   				   struct pstore_info *psi);
> >   
> > -/* Called during module_init() */
> > -extern void __init pstore_choose_compression(void);
> > -
> >   #endif
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index f09066db2d4d..e4737a5fa68c 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -274,36 +274,56 @@ static int pstore_decompress(void *in, void *out,
> >   
> >   static void allocate_buf_for_compression(void)
> >   {
> > +	struct crypto_comp *ctx;
> > +	int size;
> > +	char *buf;
> > +
> > +	/* Skip if not built-in or compression backend not selected yet. */
> >   	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
> >   		return;
> >   
> > +	/* Skip if no pstore backend yet or compression init already done. */
> > +	if (!psinfo || tfm)
> > +		return;
> > +
> >   	if (!crypto_has_comp(zbackend->name, 0, 0)) {
> > -		pr_err("No %s compression\n", zbackend->name);
> > +		pr_err("Unknown compression: %s\n", zbackend->name);
> >   		return;
> >   	}
> >   
> > -	big_oops_buf_sz = zbackend->zbufsize(psinfo->bufsize);
> > -	if (big_oops_buf_sz <= 0)
> > +	size = zbackend->zbufsize(psinfo->bufsize);
> > +	if (size <= 0) {
> > +		pr_err("Invalid compression size for %s: %d\n",
> > +		       zbackend->name, size);
> >   		return;
> > +	}
> >   
> > -	big_oops_buf = kmalloc(big_oops_buf_sz, GFP_KERNEL);
> > -	if (!big_oops_buf) {
> > -		pr_err("allocate compression buffer error!\n");
> > +	buf = kmalloc(size, GFP_KERNEL);
> > +	if (!buf) {
> > +		pr_err("Failed %d byte compression buffer allocation for: %s\n",
> > +		       size, zbackend->name);
> >   		return;
> >   	}
> >   
> > -	tfm = crypto_alloc_comp(zbackend->name, 0, 0);
> > -	if (IS_ERR_OR_NULL(tfm)) {
> > -		kfree(big_oops_buf);
> > -		big_oops_buf = NULL;
> > -		pr_err("crypto_alloc_comp() failed!\n");
> > +	ctx = crypto_alloc_comp(zbackend->name, 0, 0);
> > +	if (IS_ERR_OR_NULL(ctx)) {
> > +		kfree(buf);
> > +		pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
> > +		       PTR_ERR(ctx));
> >   		return;
> >   	}
> > +
> > +	/* A non-NULL big_oops_buf indicates compression is available. */
> > +	tfm = ctx;
> > +	big_oops_buf_sz = size;
> > +	big_oops_buf = buf;
> > +
> > +	pr_info("Using compression: %s\n", zbackend->name);
> >   }
> >   
> >   static void free_buf_for_compression(void)
> >   {
> > -	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && !IS_ERR_OR_NULL(tfm))
> > +	if (IS_ENABLED(CONFIG_PSTORE_COMPRESS) && tfm)
> >   		crypto_free_comp(tfm);
> >   	kfree(big_oops_buf);
> >   	big_oops_buf = NULL;
> > @@ -764,31 +784,28 @@ static void pstore_timefunc(struct timer_list *unused)
> >   			  jiffies + msecs_to_jiffies(pstore_update_ms));
> >   }
> >   
> > -void __init pstore_choose_compression(void)
> > +static int __init pstore_compression_late_init(void)
> >   {
> >   	const struct pstore_zbackend *step;
> >   
> >   	if (!compress)
> > -		return;
> > +		return 0;
> >   
> > +	/* Locate desired compression method. */
> >   	for (step = zbackends; step->name; step++) {
> >   		if (!strcmp(compress, step->name)) {
> >   			zbackend = step;
> > -			pr_info("using %s compression\n", zbackend->name);
> > -			return;
> > +			break;
> >   		}
> >   	}
> > -}
> >   
> > -static int __init pstore_compression_late_init(void)
> > -{
> >   	/*
> > -	 * Check if any pstore backends registered earlier but did not allocate
> > -	 * for compression because crypto was not ready, if so then initialize
> > -	 * compression.
> > +	 * Check if any pstore backends registered earlier but did not
> > +	 * initialize compression because crypto was not ready. If so,
> > +	 * then initialize compression now.
> >   	 */
> > -	if (psinfo && !tfm)
> > -		allocate_buf_for_compression();
> > +	allocate_buf_for_compression();
>
> We can also get rid of the 'zbackend' global variable since choosing the
> compression backend and allocating the buffers are done at the same time?
>
> Otherwise looks good to me,
>
> Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>

Tested this on top of Joel's patch, works fine on getting early crash 
pstore dmesg logs, thanks.

Tested-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation