[Cluster-devel] [GFS2 PATCH 2/2 v2] gfs2: change gfs2 readdir cookie

[Benjamin Marzinski]

On Wed, Aug 26, 2015 at 11:58:24AM +0100, Steven Whitehouse wrote:
> Hi,
> >@@ -1218,10 +1220,10 @@ static int compare_dents(const void *a, const void *b)
> >  	int ret = 0;
> >  	dent_a = *(const struct gfs2_dirent **)a;
> >-	hash_a = be32_to_cpu(dent_a->de_hash);
> >+	hash_a = dent_a->de_cookie;
> Does the cookie need endianess conversion? I'd suggest using sparse to check
> that you've not missed any of these.
> 
Since we don't trust the on disk value for this, and recompute it every time, we
can get by without worrying about the endianness. If we did do endian
conversion, we could skip the recomputing step, but only on filesystems
that were never used by gfs2 versions older than this patch, or some of
the cookies would be garbage.  I tested how long computing these take,
and it's not a noticeable amount, so there's really no push for saving
them.

> >  	dent_b = *(const struct gfs2_dirent **)b;
> >-	hash_b = be32_to_cpu(dent_b->de_hash);
> >+	hash_b = dent_b->de_cookie;
> >  	if (hash_a > hash_b)
> >  		ret = 1;


> >@@ -278,6 +282,12 @@ int gfs2_mount_args(struct gfs2_args *args, char
> >*options) case Opt_norgrplvb: args->ar_rgrplvb = 0; break; +
> >case Opt_loccookie: +			args->ar_loccookie = 1;
> >+			break; +		case Opt_noloccookie: +
> >args->ar_loccookie = 0; +			break;
> Hmm. Is there some way we can make it the default, without needing to
> do this? The on-disk format should still be backwards compatible I
> think? Do we need any fsck/gfs2-utils updates too?
> 
The problem is that every node in the cluster must be running this code
(well, actually, they need to be using at least by previous patch), or
things will go wrong. The location based cookies depend on my previous
patch, that copies the dirents to the same offset, when directory leaf
blocks are split. If any node creates a directory entry that splits a
leaf block, and isn't running that code, the location based cookies for
all the copied dirents get changed.  Because of this, we can't make
location cookies the default right away.

On a related note, I do think there might be a way of using the
superblock lvb or possibly a new file pointed to by the superblock for
the nodes to share this sort of compatibility data, so that we could
know if it was safe to enable certain features.
> >  		case Opt_error:
> >  		default:
> >  			pr_warn("invalid mount option: %s\n", o);



> >@@ -304,11 +306,12 @@ struct gfs2_dirent {
> >  	__be16 de_rec_len;
> >  	__be16 de_name_len;
> >  	__be16 de_type;
> >+	__be16 de_rahead;
> >  	union {
> >-		__u8 __pad[14];
> >+		__u8 __pad[12];
> >  		struct {
> >-			__be16 de_rahead;
> >-			__u8 pad2[12];
> >+			__u32 de_cookie; /* ondisk value not used */
> This is an ondisk structure so it should be __be32 to match the rest of the
> structure.

I can do that for consistency, but since we never read the value off disk, it
isn't necessary.

> 
> >+			__u8 pad3[8];
> >  		};
> >  	};
> >  };
> 
> Otherwise looks good. It will need plenty of testing though with lots of
> corner cases to ensure that nothing has been missed,
> 
> Steve.