Re: [PATCH 3/3] Safeguard against writing to an active device of another node

[NeilBrown <neilb@suse.com>]

On Wed, 29 Jul 2015 18:41:37 +0800 Guoqing Jiang <gqJiang@suse.com>
wrote:

> Hi Neil,
> 
> NeilBrown wrote:
> > On Mon,  6 Jul 2015 16:52:12 +0800 Guoqing Jiang <gqjiang@suse.com>
> > wrote:
> >
> >   
> >> Modifying an exiting device's superblock or creating a new superblock
> >> on an existing device needs to be checked because the device could be
> >> in use by another node in another array. So, we check this by taking
> >> all superblock locks in userspace so that we don't  step onto an active
> >> device used by another node and safeguard against accidental edits.
> >> After the edit is complete, we release all locks and the lockspace so
> >> that it can be used by the kernel space.
> >>
> >> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >> Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
> >>     
> >
> > Hi,
> >  thanks for these.
> >  I've applied 1 and 2.
> >
> >  Could you re-do this on to follow the kernel style of #ifdefs,
> >  so that the #ifdefs only appear in header files.
> >  i.e. when NO_DLM is defined, is_clustered() always evaluates to zero
> >  and clustered_get_dlmlock() always succeeds etc.
> >   
> Thanks, I guess you mean the following incremental modification. Please
> let me know if I misunderstood.
> 
> diff --git a/mdadm.h b/mdadm.h
> index 59f851e..4d9e8c8 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1429,9 +1429,26 @@ extern char *fd2devnm(int fd);
> 
> extern int in_initrd(void);
> extern int get_cluster_name(char **name);
> +#ifndef NO_DLM
> extern int is_clustered(struct supertype *st);
> extern int cluster_get_dlmlock(struct supertype *st, int *lockid);
> extern int cluster_release_dlmlock(struct supertype *st, int lockid);
> +#else
> +static inline int is_clustered(struct supertype *st)
> +{
> + return 0;
> +}
> +
> +static inline int cluster_get_dlmlock(struct supertype *st, int *lockid)
> +{
> + return 0;
> +}
> +
> +static inline int cluster_release_dlmlock(struct supertype *st, int lockid)
> +{
> + return 0;
> +}
> +#endif
> 
> #define _ROUND_UP(val, base) (((val) + (base) - 1) & ~(base - 1))
> #define ROUND_UP(val, base) _ROUND_UP(val, (typeof(val))(base))
> diff --git a/super1.c b/super1.c
> index c49a3b3..bd88c36 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1072,10 +1072,9 @@ static int update_super1(struct supertype *st,
> struct mdinfo *info,
> * ignored.
> */
> int rv = 0;
> + int lockid;
> struct mdp_superblock_1 *sb = st->sb;
> 
> -#ifndef NO_DLM
> - int lockid;
> if (is_clustered(st)) {
> rv = cluster_get_dlmlock(st, &lockid);
> if (rv) {
> @@ -1084,7 +1083,6 @@ static int update_super1(struct supertype *st,
> struct mdinfo *info,
> return rv;
> }
> }
> -#endif
> 
> if (strcmp(update, "homehost") == 0 &&
> homehost) {
> @@ -1342,10 +1340,9 @@ static int update_super1(struct supertype *st,
> struct mdinfo *info,
> rv = -1;
> 
> sb->sb_csum = calc_sb_1_csum(sb);
> -#ifndef NO_DLM
> if (is_clustered(st))
> cluster_release_dlmlock(st, lockid);
> -#endif
> +
> return rv;
> }
> 
> @@ -1449,9 +1446,8 @@ static int add_to_super1(struct supertype *st,
> mdu_disk_info_t *dk,
> struct mdp_superblock_1 *sb = st->sb;
> __u16 *rp = sb->dev_roles + dk->number;
> struct devinfo *di, **dip;
> -
> -#ifndef NO_DLM
> int rv, lockid;
> +
> if (is_clustered(st)) {
> rv = cluster_get_dlmlock(st, &lockid);
> if (rv) {
> @@ -1460,7 +1456,7 @@ static int add_to_super1(struct supertype *st,
> mdu_disk_info_t *dk,
> return rv;
> }
> }
> -#endif
> +
> if ((dk->state & 6) == 6) /* active, sync */
> *rp = __cpu_to_le16(dk->raid_disk);
> else if ((dk->state & ~2) == 0) /* active or idle -> spare */
> @@ -1487,10 +1483,9 @@ static int add_to_super1(struct supertype *st,
> mdu_disk_info_t *dk,
> di->next = NULL;
> *dip = di;
> 
> -#ifndef NO_DLM
> if (is_clustered(st))
> cluster_release_dlmlock(st, lockid);
> -#endif
> +
> return 0;
> }
> #endif
> @@ -1504,9 +1499,8 @@ static int store_super1(struct supertype *st, int fd)
> struct align_fd afd;
> int sbsize;
> unsigned long long dsize;
> -
> -#ifndef NO_DLM
> int rv, lockid;
> +
> if (is_clustered(st)) {
> rv = cluster_get_dlmlock(st, &lockid);
> if (rv) {
> @@ -1515,7 +1509,6 @@ static int store_super1(struct supertype *st, int fd)
> return rv;
> }
> }
> -#endif
> 
> if (!get_dev_size(fd, NULL, &dsize))
> return 1;
> @@ -1576,10 +1569,9 @@ static int store_super1(struct supertype *st, int fd)
> }
> }
> fsync(fd);
> -#ifndef NO_DLM
> if (is_clustered(st))
> cluster_release_dlmlock(st, lockid);
> -#endif
> +
> return 0;
> }
> 
> @@ -2329,7 +2321,6 @@ static int write_bitmap1(struct supertype *st, int
> fd, enum bitmap_update update
> 
> static void free_super1(struct supertype *st)
> {
> -#ifndef NO_DLM
> int rv, lockid;
> if (is_clustered(st)) {
> rv = cluster_get_dlmlock(st, &lockid);
> @@ -2339,7 +2330,7 @@ static void free_super1(struct supertype *st)
> return;
> }
> }
> -#endif
> +
> if (st->sb)
> free(st->sb);
> while (st->info) {
> @@ -2350,10 +2341,8 @@ static void free_super1(struct supertype *st)
> free(di);
> }
> st->sb = NULL;
> -#ifndef NO_DLM
> if (is_clustered(st))
> cluster_release_dlmlock(st, lockid);
> -#endif
> }
> 
> #ifndef MDASSEMBLE
> 
> Thanks,
> Guoqing

Probably something like that - yes.  Unfortunately your mailer messed
that up more that usual make it rather difficult to apply.

I was wondering if we could make it a run-time dependency though .. a
bit like get_cluster_name.  We can still do that later I guess.  I'm
not really sure what is best at the moment.

Thanks,
NeilBrown