[Cluster-devel] [GFS2 PATCH] GFS2: Set log descriptor type for jdata blocks

[Cluster-devel] [GFS2 PATCH] GFS2: Set log descriptor type for jdata blocks

[Bob Peterson]

Hi,

This patch sets the log descriptor type according to whether the
journal commit is for (journaled) data or metadata. This was
recently broken when the functions to process data and metadata
log ops were combined.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
---
 fs/gfs2/lops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 0457768..33f18b7 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -435,7 +435,9 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
 		if (total > limit)
 			num = limit;
 		gfs2_log_unlock(sdp);
-		page = gfs2_get_log_desc(sdp, GFS2_LOG_DESC_METADATA, num + 1, num);
+		page = gfs2_get_log_desc(sdp,
+					 is_databuf ? GFS2_LOG_DESC_JDATA :
+					 GFS2_LOG_DESC_METADATA, num + 1, num);
 		ld = page_address(page);
 		gfs2_log_lock(sdp);
 		ptr = (__be64 *)(ld + 1);

[Cluster-devel] [GFS2 PATCH] GFS2: Set log descriptor type for jdata blocks

[Steven Whitehouse]

Hi,

On Fri, 2013-05-24 at 15:02 -0400, Bob Peterson wrote:
> Hi,
> 
> This patch sets the log descriptor type according to whether the
> journal commit is for (journaled) data or metadata. This was
> recently broken when the functions to process data and metadata
> log ops were combined.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
Thanks - looks good. If we can automatically detect the erroneous
entries, could we also automatically deal with those too? That might be
useful in case people already have such entries in their logs,

Steve.

> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> ---
>  fs/gfs2/lops.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 0457768..33f18b7 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -435,7 +435,9 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
>  		if (total > limit)
>  			num = limit;
>  		gfs2_log_unlock(sdp);
> -		page = gfs2_get_log_desc(sdp, GFS2_LOG_DESC_METADATA, num + 1, num);
> +		page = gfs2_get_log_desc(sdp,
> +					 is_databuf ? GFS2_LOG_DESC_JDATA :
> +					 GFS2_LOG_DESC_METADATA, num + 1, num);
>  		ld = page_address(page);
>  		gfs2_log_lock(sdp);
>  		ptr = (__be64 *)(ld + 1);
> 

[Cluster-devel] [GFS2 PATCH] GFS2: Set log descriptor type for jdata blocks

[Bob Peterson]

----- Original Message -----
| > This patch sets the log descriptor type according to whether the
| > journal commit is for (journaled) data or metadata. This was
| > recently broken when the functions to process data and metadata
| > log ops were combined.
| Thanks - looks good. If we can automatically detect the erroneous
| entries, could we also automatically deal with those too? That might be
| useful in case people already have such entries in their logs,
| 
| Steve.

Hi,

Since the bad entries are data blocks, there's no good way to
distinguish them from other kinds of corrupt metadata. We could
make the code treat them as jdata rather than metadata instead of
throwing the error, but I question whether it's worth it, since the
problem will only present itself upon journal replay, and only on
recent upstream kernels. The problem doesn't exist in RHEL6 or below.
Removing our error checking in favor of treating the blocks as jdata
would make it less effective at finding real metadata block
corruption in the journals, and that seems like a check we should
keep. Still, it's simple enough to do. Opinions?

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [GFS2 PATCH] GFS2: Set log descriptor type for jdata blocks

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| On Fri, 2013-05-24 at 15:02 -0400, Bob Peterson wrote:
| > Hi,
| > 
| > This patch sets the log descriptor type according to whether the
| > journal commit is for (journaled) data or metadata. This was
| > recently broken when the functions to process data and metadata
| > log ops were combined.
| > 
| > Regards,
| > 
| > Bob Peterson
| > Red Hat File Systems
| > 
| Thanks - looks good. If we can automatically detect the erroneous
| entries, could we also automatically deal with those too? That might be
| useful in case people already have such entries in their logs,
| 
| Steve.

Hi,

Don't consider this a patch submission quite yet, but:
I wrote the following patch to do what you're suggesting. It detects the
bad blocks in the journal and adjusts accordingly. I've tested this with
both sets of corrupt metadata I have in my possession, and instrumentation
shows that it works properly.

Is this the kind of thing you had in mind? Is there a better way
to do this? Or do you have any suggestions for improvements?

Regards,

Bob Peterson
---
 fs/gfs2/lops.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 33f18b7..5a34e7e 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -31,6 +31,10 @@
 #include "util.h"
 #include "trace_gfs2.h"
 
+static int databuf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
+				    struct gfs2_log_descriptor *ld,
+				    __be64 *ptr, int pass);
+
 /**
  * gfs2_pin - Pin a buffer in memory
  * @sdp: The superblock
@@ -530,6 +534,23 @@ static void buf_lo_before_scan(struct gfs2_jdesc *jd,
 	sdp->sd_replayed_blocks = 0;
 }
 
+static bool is_mismarked_databuf(struct gfs2_sbd *sdp, __be64 *ptr,
+				 unsigned int blks)
+{
+	u64 sb_addr = GFS2_SB_ADDR >> sdp->sd_fsb2bb_shift;
+
+	while (blks) {
+		if (be64_to_cpu(*ptr) <= sb_addr)
+			return false;
+		ptr++;
+		if (be64_to_cpu(*ptr) > 1)
+			return false;
+		ptr++;
+		blks--;
+	}
+	return true;
+}
+
 static int buf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
 				struct gfs2_log_descriptor *ld, __be64 *ptr,
 				int pass)
@@ -545,6 +566,9 @@ static int buf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
 	if (pass != 1 || be32_to_cpu(ld->ld_type) != GFS2_LOG_DESC_METADATA)
 		return 0;
 
+	if (is_mismarked_databuf(sdp, ptr, blks))
+		return databuf_lo_scan_elements(jd, start, ld, ptr, pass);
+	
 	gfs2_replay_incr_blk(sdp, &start);
 
 	for (; blks; gfs2_replay_incr_blk(sdp, &start), blks--) {

[Cluster-devel] [GFS2 PATCH] GFS2: Set log descriptor type for jdata blocks

[Steven Whitehouse]

Hi,

Yes, that is the kind of thing,

Steve.

On Thu, 2013-05-30 at 11:40 -0400, Bob Peterson wrote:
> ----- Original Message -----
> | Hi,
> | 
> | On Fri, 2013-05-24 at 15:02 -0400, Bob Peterson wrote:
> | > Hi,
> | > 
> | > This patch sets the log descriptor type according to whether the
> | > journal commit is for (journaled) data or metadata. This was
> | > recently broken when the functions to process data and metadata
> | > log ops were combined.
> | > 
> | > Regards,
> | > 
> | > Bob Peterson
> | > Red Hat File Systems
> | > 
> | Thanks - looks good. If we can automatically detect the erroneous
> | entries, could we also automatically deal with those too? That might be
> | useful in case people already have such entries in their logs,
> | 
> | Steve.
> 
> Hi,
> 
> Don't consider this a patch submission quite yet, but:
> I wrote the following patch to do what you're suggesting. It detects the
> bad blocks in the journal and adjusts accordingly. I've tested this with
> both sets of corrupt metadata I have in my possession, and instrumentation
> shows that it works properly.
> 
> Is this the kind of thing you had in mind? Is there a better way
> to do this? Or do you have any suggestions for improvements?
> 
> Regards,
> 
> Bob Peterson
> ---
>  fs/gfs2/lops.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 33f18b7..5a34e7e 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -31,6 +31,10 @@
>  #include "util.h"
>  #include "trace_gfs2.h"
>  
> +static int databuf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
> +				    struct gfs2_log_descriptor *ld,
> +				    __be64 *ptr, int pass);
> +
>  /**
>   * gfs2_pin - Pin a buffer in memory
>   * @sdp: The superblock
> @@ -530,6 +534,23 @@ static void buf_lo_before_scan(struct gfs2_jdesc *jd,
>  	sdp->sd_replayed_blocks = 0;
>  }
>  
> +static bool is_mismarked_databuf(struct gfs2_sbd *sdp, __be64 *ptr,
> +				 unsigned int blks)
> +{
> +	u64 sb_addr = GFS2_SB_ADDR >> sdp->sd_fsb2bb_shift;
> +
> +	while (blks) {
> +		if (be64_to_cpu(*ptr) <= sb_addr)
> +			return false;
> +		ptr++;
> +		if (be64_to_cpu(*ptr) > 1)
> +			return false;
> +		ptr++;
> +		blks--;
> +	}
> +	return true;
> +}
> +
>  static int buf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
>  				struct gfs2_log_descriptor *ld, __be64 *ptr,
>  				int pass)
> @@ -545,6 +566,9 @@ static int buf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
>  	if (pass != 1 || be32_to_cpu(ld->ld_type) != GFS2_LOG_DESC_METADATA)
>  		return 0;
>  
> +	if (is_mismarked_databuf(sdp, ptr, blks))
> +		return databuf_lo_scan_elements(jd, start, ld, ptr, pass);
> +	
>  	gfs2_replay_incr_blk(sdp, &start);
>  
>  	for (; blks; gfs2_replay_incr_blk(sdp, &start), blks--) {