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

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

On Fri, 2017-01-20 at 16:00 -0500, Linda Knippers wrote:
> Hi Vishal,
> 
> It's nice to see a BTT check utility.
> 
> I have a few high level questions/comments below.

Hi Linda - thanks! See below -

> 
> -- ljk
> 
> On 01/19/2017 10:12 PM, Vishal Verma wrote:
> > 
> > +
> > +EXAMPLES
> > +--------
> > +
> > +Check a BTT namespace
> > +[verse]
> > +ndctl disable-namespace namespace0.0
> > +ndctl check-namespace namespace0.0
> 
> If the namespace has to be disabled to be checked, it's probably worth
> saying in
> the text as well as in the example.

Yes, updating.

> 
> > +
> > +Check a BTT namespace, but don't actually restore/correct anything
> > +[verse]
> > +ndctl check-namespace --dry-run
> 
> Does the namespace not have to be disabled for the --dry-run option?

It does, and I'm reversing the logic, converting --dry-run to --repair
according to Dan's suggestion, but I'll include the disable-namespace in
both examples.

> 
> 
> If we only know how to check namespaces that are in sector mode,
> should you check up front and then bail out with an appropriate
> message
> if the namespace isn't in sector mode?  Otherwise, it just seems
> like we fail later with a message that isn't very helpful.

Like I said in a reply to Jeff, it isn't really possible for ndctl to
tell if it was supposed to be in sector mode, only the kernel can do
that during the probe. I think the best we can do is notify that a BTT
was not found on the namespace at all (which is I believe what we do
here).

> 
> > +
> > +	printf("checking %s\n", devname);
> > +	if (!is_namespace_offline(ndns)) {
> > +		error("%s: check aborted, namespace online\n",
> > devname);
> > +		return -EBUSY;
> > +	}
> > +
> > +	raw_mode = ndctl_namespace_get_raw_mode(ndns);
> > +	ndctl_namespace_set_raw_mode(ndns, 1);
> > +	rc = ndctl_namespace_enable(ndns);
> > +	if (rc != 0)
> > +		error("%s: failed to enable raw mode\n", devname);
> 
> The namespace is now enabled in raw mode.  How do we prevent something
> else from seeing the device and opening it?  You open it later but not
> in exclusive mode, and it gets opened and closed a few times leaving
> windows where something else could open it.  Or is something else
> protecting the device?

Good point, like Dan also suggested, I'll change this to doing one
O_EXCL open at the start and using that fd for all subsequent
operations.

> 
> > +	ndctl_namespace_set_raw_mode(ndns, raw_mode);
> 
> What is this second call doing?

See the conversation with Jeff regarding these - we're just saving the
raw_mode, setting it to '1', and restoring raw_mode on failure. Like
Jeff suggested, I'll move the restore to the 'out' label.

> 
> > +	sprintf(path, "/dev/%s",
> > ndctl_namespace_get_block_device(ndns));
> > +
> > +	btt_sb = malloc(sizeof(*btt_sb));
> > +	if (btt_sb == NULL) {
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	bttc = calloc(1, sizeof(*bttc));
> > +	if (bttc == NULL) {
> > +		rc = -ENOMEM;
> > +		goto out_sb;
> > +	}
> > +	bttc->path = path;
> > +	bttc->rawsize = ndctl_namespace_get_size(ndns);
> > +	ndctl_namespace_get_uuid(ndns, bttc->parent_uuid);
> > +
> > +	rc = btt_read_info(bttc, btt_sb, 0);
> > +	if (rc)
> > +		goto out_bttc;
> > +	rc = btt_info_verify(bttc, btt_sb);
> > +	if (rc) {
> > +		rc = btt_recover_first_sb(bttc);
> > +		if (rc) {
> > +			error("Unable to recover any BTT info
> > blocks, aborting\n");
> > +			goto out_bttc;
> > +		}
> > +		rc = btt_read_info(bttc, btt_sb, 0);
> > +		if (rc)
> > +			goto out_bttc;
> > +	}
> > +	rc = btt_discover_arenas(bttc);
> > +	if (rc)
> > +		goto out_bttc;
> > +
> > +	rc = btt_create_mappings(bttc);
> > +	if (rc)
> > +		goto out_bttc;
> > +
> > +	rc = btt_check_arenas(bttc);
> > +
> > + out_bttc:
> > +	btt_remove_mappings(bttc);
> > +	free(bttc);
> > + out_sb:
> > +	free(btt_sb);
> > + out:
> > +	ndctl_namespace_disable(ndns);
> 
> Since you put the namespace into raw mode, do you have to put it back
> into sector mode?

Yep will fix.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm