From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bob Peterson <rpeterso@redhat.com>
Date: Thu, 2 Jul 2015 10:38:19 -0400 (EDT)
Subject: [Cluster-devel] [fsck.gfs2 PATCH 2 of 2] fsck.gfs2: Detect,
 fix and clone duplicate block refs within a dinode
In-Reply-To: <559540FE.6030606@redhat.com>
References: <324505055.28858416.1435762387600.JavaMail.zimbra@redhat.com>
	<559540FE.6030606@redhat.com>
Message-ID: <209596538.30005255.1435847899356.JavaMail.zimbra@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

----- Original Message -----
> Hi Bob,
> 
> A couple of comments in line, otherwise both patches look good to me.
(snip)
> > +	*bh = bread(ip->i_sbd, block);
> 
> Does this need a NULL check, or does check_metatree handle that?

This doesn't need a NULL check. It's handled properly in function
build_and_check_metalist, which, by design, plans on either getting
or not getting a proper bh pointer.

(snip)
> > +static void clone_dup_ref_in_inode(struct gfs2_inode *ip, struct duptree
> > *dt)
> > +{
> > +	int error;
> > +	struct clone_target clonet = {.dup_block = dt->block, .first = 1};
> > +	struct metawalk_fxns pass1b_fxns_clone = {
> > +		.private = &clonet,
> > +		.check_metalist = clone_check_meta,
> > +		.check_data = clone_data,
> > +	};
> > +
> > +	error = check_metatree(ip, &pass1b_fxns_clone);
> > +	if (error) {
> > +		log_err(_("Error cloning duplicate reference(s) to block %lld "
> > +			  "(0x%llx).\n"), (unsigned long long)dt->block,
> > +			(unsigned long long)dt->block);
> > +	}
> > +}
> 
> Void functions with error paths always ring alarm bells in my head,
> should this one return the error value?
> 
> Andy

You've got a good point, and maybe we should add some error checking,
but to be honest, there's not much we can do about an error at this
point.

First of all, in this code path, errors seem unlikely because
(a) clone_check_meta always returns 0, and,
(b) function clone_data takes errors in the data blocks into account
and if it can't fix them, will zero out the reference to them.

So any errors from function check_metatree() would be due to some
kind of strange unforeseen problem, such as metadata corruption.
Since this function is in pass1b, all metadata should have been
validated in pass1 at the point where this function runs.
In that case, there's not much to be done but log it (which it does)
and move on.

When it returns from function clone_dup_ref_in_inode(), it sets the
block_map appropriately for the duplicated block, so it's done about
all it can about the situation, and certainly more than it did prior
to the patch.

I've run this patch against more than 200 metadata sets with varying
degrees of brokenness, and it seems pretty robust. We can always
amend the code in the future if we find something that warrants it.

Thanks for reviewing the patch.

Regards,

Bob Peterson
Red Hat File Systems