[Cluster-devel] [fsck.gfs2 PATCH 2 of 2] fsck.gfs2: Detect, fix and clone duplicate block refs within a dinode

Hi Bob,

A couple of comments in line, otherwise both patches look good to me.

On 01/07/15 15:53, Bob Peterson wrote:
> Hi,
>
> Prior to this patch, fsck.gfs2 was unable to detect and fix duplicate
> block references within the same file. This patch detects when data
> blocks are duplicated within a dinode, then tries to clone the data
> to a new block.
>
> Regards,
>
> Bob Peterson
>
> Signed-off-by: Bob Peterson <[email protected]>
> ---
<snip>

> diff --git a/gfs2/fsck/pass1b.c b/gfs2/fsck/pass1b.c
> index a8f3d28..c1598ff 100644
> --- a/gfs2/fsck/pass1b.c
> +++ b/gfs2/fsck/pass1b.c
> @@ -28,6 +28,11 @@ struct dup_handler {
>   	int ref_count;
>   };
>
> +struct clone_target {
> +	uint64_t dup_block;
> +	int first;
> +};
> +
>   static void log_inode_reference(struct duptree *dt, osi_list_t *tmp, int inval)
>   {
>   	char reftypestring[32];
> @@ -249,6 +254,116 @@ static void revise_dup_handler(uint64_t dup_blk, struct dup_handler *dh)
>   	}
>   }
>
> +static int clone_check_meta(struct gfs2_inode *ip, uint64_t block,
> +			    struct gfs2_buffer_head **bh, int h,
> +			    int *is_valid, int *was_duplicate, void *private)
> +{
> +	*was_duplicate = 0;
> +	*is_valid = 1;
> +	*bh = bread(ip->i_sbd, block);

Does this need a NULL check, or does check_metatree handle that?

> +	return 0;
> +}
> +
> +/* clone_data - clone a duplicate reference
> + *
> + * This function remembers the first reference to the specified block, and
> + * clones all subsequent references to it (with permission).
> + */
> +static int clone_data(struct gfs2_inode *ip, uint64_t metablock,
> +		      uint64_t block, void *private,
> +		      struct gfs2_buffer_head *bh, uint64_t *ptr)
> +{
> +	struct clone_target *clonet = (struct clone_target *)private;
> +	struct gfs2_buffer_head *clone_bh;
> +	uint64_t cloneblock;
> +	int error;
> +
> +	if (block != clonet->dup_block)
> +		return 0;
> +
> +	if (clonet->first) {
> +		log_debug(_("Inode %lld (0x%llx)'s first reference to "
> +			    "block %lld (0x%llx) is targeted for cloning.\n"),
> +			  (unsigned long long)ip->i_di.di_num.no_addr,
> +			  (unsigned long long)ip->i_di.di_num.no_addr,
> +			  (unsigned long long)block,
> +			  (unsigned long long)block);
> +		clonet->first = 0;
> +		return 0;
> +	}
> +	log_err(_("Error: Inode %lld (0x%llx)'s subsequent reference to "
> +		  "block %lld (0x%llx) is an error.\n"),
> +		(unsigned long long)ip->i_di.di_num.no_addr,
> +		(unsigned long long)ip->i_di.di_num.no_addr,
> +		(unsigned long long)block, (unsigned long long)block);
> +	if (query( _("Okay to clone the duplicated reference? (y/n) "))) {
> +		error = lgfs2_meta_alloc(ip, &cloneblock);
> +		if (!error) {
> +			clone_bh = bread(ip->i_sbd, clonet->dup_block);
> +			if (clone_bh) {
> +				fsck_blockmap_set(ip, cloneblock, _("data"),
> +						  GFS2_BLKST_USED);
> +				clone_bh->b_blocknr = cloneblock;
> +				bmodified(clone_bh);
> +				brelse(clone_bh);
> +				/* Now fix the reference: */
> +				*ptr = cpu_to_be64(cloneblock);
> +				bmodified(bh);
> +				log_err(_("Duplicate reference to block %lld "
> +					  "(0x%llx) was cloned to block %lld "
> +					  "(0x%llx).\n"),
> +					(unsigned long long)block,
> +					(unsigned long long)block,
> +					(unsigned long long)cloneblock,
> +					(unsigned long long)cloneblock);
> +				return 0;
> +			}
> +		}
> +		log_err(_("Error: Unable to allocate a new data block.\n"));
> +		if (!query("Should I zero the reference instead? (y/n)")) {
> +			log_err(_("Duplicate reference to block %lld "
> +				  "(0x%llx) was not fixed.\n"),
> +				(unsigned long long)block,
> +				(unsigned long long)block);
> +			return 0;
> +		}
> +		*ptr = 0;
> +		bmodified(bh);
> +		log_err(_("Duplicate reference to block %lld (0x%llx) was "
> +			  "zeroed.\n"),
> +			(unsigned long long)block,
> +			(unsigned long long)block);
> +	} else {
> +		log_err(_("Duplicate reference to block %lld (0x%llx) "
> +			  "was not fixed.\n"), (unsigned long long)block,
> +			(unsigned long long)block);
> +	}
> +	return 0;
> +}
> +
> +/* clone_dup_ref_in_inode - clone a duplicate reference within a single inode
> + *
> + * This function traverses the metadata tree of an inode, cloning all
> + * but the first reference to a duplicate block reference.
> + */
> +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

> +
>   /* handle_dup_blk - handle a duplicate block reference.
>    *
>    * This function should resolve and delete the duplicate block reference given,
> @@ -389,6 +504,9 @@ static int handle_dup_blk(struct gfs2_sbd *sdp, struct duptree *dt)
>   			   (unsigned long long)id->block_no);
>   		ip = fsck_load_inode(sdp, id->block_no);
>
> +		if (dt->dup_flags & DUPFLAG_REF1_IS_DUPL)
> +			clone_dup_ref_in_inode(ip, dt);
> +
>   		q = block_type(id->block_no);
>   		if (q == GFS2_BLKST_UNLINKED) {
>   			log_debug( _("The remaining reference inode %lld "
> @@ -468,7 +586,8 @@ static int check_metalist_refs(struct gfs2_inode *ip, uint64_t block,
>   }
>
>   static int check_data_refs(struct gfs2_inode *ip, uint64_t metablock,
> -			   uint64_t block, void *private)
> +			   uint64_t block, void *private,
> +			   struct gfs2_buffer_head *bh, uint64_t *ptr)
>   {
>   	return add_duplicate_ref(ip, block, ref_as_data, 1, INODE_VALID);
>   }
> diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
> index b31fbd4..900d4e1 100644
> --- a/gfs2/fsck/pass2.c
> +++ b/gfs2/fsck/pass2.c
> @@ -1580,7 +1580,8 @@ static int check_metalist_qc(struct gfs2_inode *ip, uint64_t block,
>   }
>
>   static int check_data_qc(struct gfs2_inode *ip, uint64_t metablock,
> -			 uint64_t block, void *private)
> +			 uint64_t block, void *private,
> +			 struct gfs2_buffer_head *bbh, uint64_t *ptr)
>   {
>   	struct gfs2_buffer_head *bh;
>
> diff --git a/gfs2/fsck/util.c b/gfs2/fsck/util.c
> index 4022fee..a6a5cdc 100644
> --- a/gfs2/fsck/util.c
> +++ b/gfs2/fsck/util.c
> @@ -339,15 +339,16 @@ int add_duplicate_ref(struct gfs2_inode *ip, uint64_t block,
>   	   resolve it. The first reference can't be the second reference. */
>   	if (id && first && !(dt->dup_flags & DUPFLAG_REF1_FOUND)) {
>   		log_info(_("Original reference to block %llu (0x%llx) was "
> -			   "previously found to be bad and deleted.\n"),
> +			   "either found to be bad and deleted, or else "
> +			   "a duplicate within the same inode.\n"),
>   			 (unsigned long long)block,
>   			 (unsigned long long)block);
>   		log_info(_("I'll consider the reference from inode %llu "
>   			   "(0x%llx) the first reference.\n"),
>   			 (unsigned long long)ip->i_di.di_num.no_addr,
>   			 (unsigned long long)ip->i_di.di_num.no_addr);
> -		dt->dup_flags |= DUPFLAG_REF1_FOUND;
> -		return meta_is_good;
> +		dt->dup_flags |= DUPFLAG_REF1_IS_DUPL;
> +		dt->refs++;
>   	}
>
>   	/* The first time this is called from pass1 is actually the second
>