On 12/18/2017 11:42 PM, Jes Sorensen wrote:
On 12/12/2017 09:00 PM, Guoqing Jiang wrote:
Previously, the dlm locking only protects several
functions which writes to superblock (update_super,
add_to_super and store_super), and we missed other
funcs such as add_internal_bitmap. We also need to
call the funcs which read superblock under the
locking protection to avoid consistent issue.
So let's remove the dlm stuffs from super1.c, and
provide the locking mechanism to the main() except
assemble mode which will be handled in next commit.
And since we can identify it is a clustered raid or
not based on check the different conditions of each
mode, so the change should not have effect on native
array.
And we improve the existed locking stuffs as follows:
1. replace ls_unlock with ls_unlock_wait since we
should return when unlock operation is complete.
2. inspired by lvm, let's also try to use the existed
lockspace first before creat a lockspace blindly if
the lockspace not released for some reason.
3. try more times before quit if EAGAIN happened for
locking.
Reviewed-by: NeilBrown <neilb@xxxxxxxx>
Signed-off-by: Guoqing Jiang <gqjiang@xxxxxxxx>
---
mdadm.c | 20 ++++++++++++++++++++
mdadm.h | 8 +++++---
super1.c | 42 -----------------------------------------
util.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
4 files changed, 80 insertions(+), 55 deletions(-)
diff --git a/mdadm.c b/mdadm.c
index 62d7ec34a17f..895a9706b1d9 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -57,6 +57,7 @@ int main(int argc, char *argv[])
struct mddev_dev *devlist = NULL;
struct mddev_dev **devlistend = & devlist;
struct mddev_dev *dv;
+ mdu_array_info_t array;
int devs_found = 0;
char *symlinks = NULL;
int grow_continue = 0;
@@ -103,6 +104,7 @@ int main(int argc, char *argv[])
FILE *outf;
int mdfd = -1;
+ int locked = 0;
srandom(time(0) ^ getpid());
@@ -1434,6 +1436,22 @@ int main(int argc, char *argv[])
/* --scan implied --brief unless -vv */
c.brief = 1;
+ if (mode == CREATE) {
+ if (s.bitmap_file && strcmp(s.bitmap_file, "clustered") == 0) {
+ locked = lock_cluster();
+ if (locked != 1)
+ exit(1);
+ }
+ } else if (mode == MANAGE || mode == GROW || mode == INCREMENTAL) {
+ if (!md_get_array_info(mdfd, &array)) {
+ if (array.state & (1 << MD_SB_CLUSTERED)) {
+ locked = lock_cluster();
+ if (locked != 1)
+ exit(1);
+ }
+ }
+ }
+
I like the switch to having more generic functions handling this rather
than open coding things too much. However the above still seems to be a
lot of multi case backwards hoops jumping.
Yes as you said, we can't earn more benefit by change it to switch.
Anything we can do to simplify this?
I find we already have the switch part after them.
switch(mode) {
case MANAGE:
/* readonly, add/remove, readwrite, runstop */
Maybe it is better to move them to above existed switch section,
what do you think about it?
+static int dlm_lockid = 0;
+int lock_cluster(void)
+{
+ if (dlm_funs_ready()) {
+ int rv;
+
+ rv = cluster_get_dlmlock(&dlm_lockid);
+ if (rv) {
+ pr_err("failed to lock cluster\n");
+ return -1;
+ }
+ return 1;
+ }
+ pr_err("Something wrong with dlm library (libdlm_lt.so.3)\n");
Please don't refer explicitly to the soname of a library like this. It
is not a good error message.
NP, I will remove it.
+ return 0;
+}
+
+void unlock_cluster(void)
+{
+ if (dlm_lockid != 0 && dlm_funs_ready()) {
+ int rv;
+
+ rv = cluster_release_dlmlock(dlm_lockid);
+ if (rv)
+ pr_err("failed to unlock cluster\n");
+ }
+}
In addition, I don't like the use of a global variable to store this.
While the app isn't multithreaded it's still ugly, why not just return
the lockid itself as an indicator? There must be a known error value for it.
Hmm, since the lockid just get the value from dlm_lock_res->lksb.sb_lkid,
so perhaps we can remove it by just use sb_lkid directly.
Thanks & Regards,
Guoqing
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
