Re: [ndctl PATCH v3 6/7] ndctl: add a BTT check utility

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Mon, 2017-03-13 at 19:26 -0400, Linda Knippers wrote:
> Hi Vishal,
> 
> Thanks for all your work on this.  I finally got a chance to do some
> testing and have a few usability suggestions for you to consider.

Thanks for the comments, Linda. See below, but yes I can send follow on
patches for these.

> 
> On 03/04/2017 01:13 AM, Vishal Verma wrote:
> <snip>
> > 
> > */
> > +	raw_mode = ndctl_namespace_get_raw_mode(ndns);
> > +
> > +	/*
> > +	 * Putting the namespace into raw mode will allow us to
> > access
> > +	 * the btt metadata.
> > +	 */
> > +	rc = ndctl_namespace_set_raw_mode(ndns, 1);
> > +	if (rc < 0) {
> > +		err(bttc, "%s: failed to set the raw mode flag:
> > %d\n",
> > +			devname, rc);
> > +		goto out_ns;
> > +	}
> 
> When I ran the utility as non-root, I got this:
> $ ndctl check-namespace namespace3.0
> namespace3.0: namespace_check: namespace3.0: failed to set the raw
> mode flag: -6
> error checking namespaces: No such device or address
> 
> I think it ought to end with Permission denied, which it would do if
> ndctl_namespace_get_raw_mode() returned with -errno rather than a
> hard
> coded ENXIO.

Good point - I'll fix this (in a follow on patch).

> 
> If that's main reason that setting the raw mode flag can fail, then
> the
> err() could be an info().  That would simplify the output.

I think it should still be an error as it is a fatal problem and we
have to abort..

<snip>

> > +
> > +	rc = btt_info_read_verify(bttc, btt_sb, bttc->start_off);
> > +	if (rc) {
> > +		rc = btt_recover_first_sb(bttc);
> > +		if (rc) {
> > +			info(bttc, "Unable to recover any BTT info
> > blocks\n");
> > +			goto out_close;
> 
> When I ran this on a namespace that never had a BTT, I thought I'd
> get a error
> basically saying that I'm screwed, but instead I got:
> 
> $ sudo ndctl check-namespace namespace1.0
> error checking namespaces: No such device or address
> 
> It seems like the info() above is rather important and should be an
> err().
> For a checker, maybe verbose should be the default anyway.

Ok - let me revisit the no BTT case and see if we can print out
something better.

> 
> > +		}
> > +		rc = btt_info_read_verify(bttc, btt_sb, bttc-
> > >start_off);
> > +		if (rc)
> > +			goto out_close;
> > +	}
> > +	rc = btt_discover_arenas(bttc);
> > +	if (rc)
> > +		goto out_close;
> > +
> > +	rc = btt_create_mappings(bttc);
> > +	if (rc)
> > +		goto out_close;
> > +
> > +	rc = btt_check_arenas(bttc);
> > +
> > +	btt_remove_mappings(bttc);
> > + out_close:
> > +	close(bttc->fd);
> > + out_sb:
> > +	free(btt_sb);
> > + out_ns:
> > +	ndctl_namespace_set_raw_mode(ndns, raw_mode);
> > +	ndctl_namespace_disable_invalidate(ndns);
> > +	if (disabled_flag)
> > +		if(ndctl_namespace_enable(ndns) < 0)
> > +			info(bttc, "%s: failed to re-enable
> > namespace\n",
> > +				devname);
> 
> I didn't get this error but it seems rather important too.

This should normally never happen, and I had it as an info because at
this point we can't really do anything about it. Any check/repair has
already completed, and if we weren't able to enable the namespace, the
user likely won't either for the same reason.. In any case, it seems
more like an informational message since nothing can be done about it,
and it doesn't affect any future flow..

> 
> Thanks,
> 
> - ljk
> 
> > + out_bttc:
> > +	free(bttc);
> > +	return rc;
> > +}
> 
> 
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm