Re: [PATCH] ndctl: add a BTT check utility

["Verma, Vishal L" <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Fri, 2017-01-20 at 18:14 -0500, Jeff Moyer wrote:

<snip>
> 
> > +static int btt_map_read(struct arena_info *a, __u32 lba, __u32
> > *mapping,
> > +			int *trim, int *error)
> > +{
> > +	__u32 raw_mapping, postmap, ze, z_flag, e_flag;
> > +
> > +	raw_mapping = le32toh(a->map.map[lba]);
> > +	z_flag = (raw_mapping & MAP_TRIM_MASK) >> MAP_TRIM_SHIFT;
> > +	e_flag = (raw_mapping & MAP_ERR_MASK) >> MAP_ERR_SHIFT;
> > +	ze = (z_flag << 1) + e_flag;
> > +	postmap = raw_mapping & MAP_LBA_MASK;
> > +
> > +	/* Reuse the {z,e}_flag variables for *trim and *error */
> > +	z_flag = 0;
> > +	e_flag = 0;
> > +
> > +	switch (ze) {
> > +	case 0:
> > +		/* Initial state. Return postmap = premap */
> > +		*mapping = lba;
> > +		break;
> > +	case 1:
> > +		*mapping = postmap;
> > +		e_flag = 1;
> > +		break;
> > +	case 2:
> > +		*mapping = postmap;
> > +		z_flag = 1;
> > +		break;
> > +	case 3:
> > +		*mapping = postmap;
> > +		break;
> > +	default:
> > +		return -EIO;
> > +	}
> > +
> > +	if (trim)
> > +		*trim = z_flag;
> > +	if (error)
> > +		*error = e_flag;
> > +
> > +	return 0;
> > +}
> 
> No caller uses z or e, so get rid of those args.  Also, your default
> entry in the switch statement can never be hit.  Fixing those, the
> function is just:
> 
> /*
>  * btt_map_lookup
>  *
>  * Given a pre-map Arena Block Address, return the post-map ABA.
>  */
> static u32 btt_map_lookup(struct arena_info *a, __u32 lba)
> {
>         return (le32toh(a->map.map[lba]) & MAP_LBA_MASK);

I think this might be an oversimplification?
We still have to check whether the z and e flags are both 0, and if so
return 'lba' -- the initial state check. This would return '0' which
would be incorrect.. I will simplify the arguments etc though, and not
return the values of flags.

> }
> 
> You can take or leave the function name change.  Please keep the
> comment, and maybe kernel-doc-ify it.
> 
> > +static int btt_map_write(struct arena_info *a, __u32 lba, __u32
> > mapping,
> > +			__u32 z_flag, __u32 e_flag)
> > +{
> 
> Again, z and e are unused.  Nuke 'em.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm