From: Mr NeilBrown Date: Thu, 18 Jun 2020 01:38:25 +0000 (+1000) Subject: LU-12780 scrub: all update to bitfields must be protected. X-Git-Tag: 2.13.55~93 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=23a6e1ed8eec2c07653ed07c35bb109ecb87a5b7 LU-12780 scrub: all update to bitfields must be protected. When a structure contains bitfields, these are updated by a read-modify-write of the whole word. If two of these updates can race, corruption can occurs - updates can be lost. To avoid this, it is best to protect updates with a spinlock. Many updates to the os_* bit fields are already protected by ->os_lock. This patch addes the lock to the remaining updates. In many cases, this only requires moving a 'lock' earlier, or an 'unlock' later. Signed-off-by: Mr NeilBrown Change-Id: I2335d08cd53dcda98d8046d730829347456a6c5d Reviewed-on: https://review.whamcloud.com/38974 Tested-by: jenkins Reviewed-by: Alex Zhuravlev Tested-by: Maloo Reviewed-by: James Simmons Reviewed-by: Oleg Drokin --- diff --git a/lustre/include/lustre_scrub.h b/lustre/include/lustre_scrub.h index 313af0f..a4eb139 100644 --- a/lustre/include/lustre_scrub.h +++ b/lustre/include/lustre_scrub.h @@ -288,6 +288,10 @@ struct lustre_scrub { __u64 os_new_checked; __u64 os_pos_current; __u32 os_start_flags; + /* Some of these bits can be set by different threads so + * all updates must be protected by ->os_lock to avoid + * racing read-modify-write cycles causing corruption. + */ unsigned int os_in_prior:1, /* process inconsistent item * found by RPC prior */ os_waiting:1, /* Waiting for scan window. */ diff --git a/lustre/osd-ldiskfs/osd_scrub.c b/lustre/osd-ldiskfs/osd_scrub.c index 5130fe7..90f273e 100644 --- a/lustre/osd-ldiskfs/osd_scrub.c +++ b/lustre/osd-ldiskfs/osd_scrub.c @@ -339,8 +339,11 @@ iget: GOTO(out, rc = 0); } - if (!scrub->os_partial_scan) + if (!scrub->os_partial_scan) { + spin_lock(&scrub->os_lock); scrub->os_full_speed = 1; + spin_unlock(&scrub->os_lock); + } switch (val) { case SCRUB_NEXT_NOLMA: @@ -367,9 +370,11 @@ iget: GOTO(out, rc = 0); } else { - if (!scrub->os_partial_scan) + if (!scrub->os_partial_scan) { + spin_lock(&scrub->os_lock); scrub->os_full_speed = 1; - + spin_unlock(&scrub->os_lock); + } sf->sf_flags |= SF_INCONSISTENT; /* XXX: If the device is restored from file-level backup, then @@ -474,6 +479,7 @@ static int osd_scrub_prep(const struct lu_env *env, struct osd_device *dev) if (flags & SS_RESET) scrub_file_reset(scrub, dev->od_uuid, 0); + spin_lock(&scrub->os_lock); if (flags & SS_AUTO_FULL) { scrub->os_full_speed = 1; scrub->os_partial_scan = 0; @@ -491,7 +497,6 @@ static int osd_scrub_prep(const struct lu_env *env, struct osd_device *dev) scrub->os_partial_scan = 0; } - spin_lock(&scrub->os_lock); scrub->os_in_prior = 0; scrub->os_waiting = 0; scrub->os_paused = 0; @@ -810,10 +815,10 @@ static int osd_scrub_next(struct osd_thread_info *info, struct osd_device *dev, oii = list_entry(scrub->os_inconsistent_items.next, struct osd_inconsistent_item, oii_list); - spin_unlock(&scrub->os_lock); *oic = &oii->oii_cache; scrub->os_in_prior = 1; + spin_unlock(&scrub->os_lock); return 0; } @@ -897,7 +902,9 @@ static int osd_scrub_exec(struct osd_thread_info *info, struct osd_device *dev, rc = osd_scrub_check_update(info, dev, oic, rc); if (rc != 0) { + spin_lock(&scrub->os_lock); scrub->os_in_prior = 0; + spin_unlock(&scrub->os_lock); return rc; } @@ -910,7 +917,9 @@ static int osd_scrub_exec(struct osd_thread_info *info, struct osd_device *dev, } if (scrub->os_in_prior) { + spin_lock(&scrub->os_lock); scrub->os_in_prior = 0; + spin_unlock(&scrub->os_lock); return 0; } @@ -971,6 +980,7 @@ static void osd_scrub_join(const struct lu_env *env, struct osd_device *dev, LASSERT(!(flags & SS_AUTO_PARTIAL)); down_write(&scrub->os_rwsem); + spin_lock(&scrub->os_lock); scrub->os_in_join = 1; if (flags & SS_SET_FAILOUT) sf->sf_param |= SP_FAILOUT; @@ -997,6 +1007,7 @@ static void osd_scrub_join(const struct lu_env *env, struct osd_device *dev, sf->sf_flags |= SF_AUTO; scrub->os_full_speed = 1; } + spin_unlock(&scrub->os_lock); scrub->os_new_checked = 0; if (sf->sf_pos_last_checkpoint != 0) @@ -2302,8 +2313,11 @@ osd_ios_ROOT_scan(struct osd_thread_info *info, struct osd_device *dev, * FID directly, instead, the OI scrub will scan the OI structure * and try to re-generate the LMA from the OI mapping. But if the * OI mapping crashed or lost also, then we have to give up under - * double failure cases. */ + * double failure cases. + */ + spin_lock(&scrub->os_lock); scrub->os_convert_igif = 1; + spin_unlock(&scrub->os_lock); child = osd_lookup_one_len_unlocked(dev, dot_lustre_name, dentry, strlen(dot_lustre_name)); if (IS_ERR(child)) { @@ -2553,7 +2567,9 @@ void osd_scrub_stop(struct osd_device *dev) /* od_otable_mutex: prevent curcurrent start/stop */ mutex_lock(&dev->od_otable_mutex); + spin_lock(&scrub->os_lock); scrub->os_paused = 1; + spin_unlock(&scrub->os_lock); scrub_stop(scrub); mutex_unlock(&dev->od_otable_mutex); } @@ -3046,6 +3062,7 @@ int osd_oii_insert(struct osd_device *dev, struct osd_idmap_cache *oic, oii->oii_cache = *oic; oii->oii_insert = insert; + spin_lock(&lscrub->os_lock); if (lscrub->os_partial_scan) { __u64 now = ktime_get_real_seconds(); @@ -3066,7 +3083,6 @@ int osd_oii_insert(struct osd_device *dev, struct osd_idmap_cache *oic, lscrub->os_full_scrub = 1; } - spin_lock(&lscrub->os_lock); if (unlikely(!thread_is_running(thread))) { spin_unlock(&lscrub->os_lock); OBD_FREE_PTR(oii); diff --git a/lustre/osd-zfs/osd_scrub.c b/lustre/osd-zfs/osd_scrub.c index d835708..9df6f74 100644 --- a/lustre/osd-zfs/osd_scrub.c +++ b/lustre/osd-zfs/osd_scrub.c @@ -196,7 +196,10 @@ zget: GOTO(out, rc); } + spin_lock(&scrub->os_lock); scrub->os_full_speed = 1; + spin_unlock(&scrub->os_lock); + sf->sf_flags |= SF_INCONSISTENT; } else if (oid == oid2) { GOTO(out, rc = 0); @@ -227,7 +230,9 @@ zget: } update: + spin_lock(&scrub->os_lock); scrub->os_full_speed = 1; + spin_unlock(&scrub->os_lock); sf->sf_flags |= SF_INCONSISTENT; } @@ -304,6 +309,7 @@ static int osd_scrub_prep(const struct lu_env *env, struct osd_device *dev) if (flags & SS_RESET) scrub_file_reset(scrub, dev->od_uuid, 0); + spin_lock(&scrub->os_lock); scrub->os_partial_scan = 0; if (flags & SS_AUTO_FULL) { scrub->os_full_speed = 1; @@ -315,7 +321,6 @@ static int osd_scrub_prep(const struct lu_env *env, struct osd_device *dev) scrub->os_full_speed = 0; } - spin_lock(&scrub->os_lock); scrub->os_in_prior = 0; scrub->os_waiting = 0; scrub->os_paused = 0; @@ -538,7 +543,9 @@ static int osd_scrub_exec(const struct lu_env *env, struct osd_device *dev, spin_unlock(&scrub->os_lock); } } else { + spin_lock(&scrub->os_lock); scrub->os_in_prior = 0; + spin_unlock(&scrub->os_lock); } if (rc) @@ -1388,7 +1395,9 @@ void osd_scrub_stop(struct osd_device *dev) /* od_otable_sem: prevent concurrent start/stop */ down(&dev->od_otable_sem); + spin_lock(&scrub->os_lock); scrub->os_paused = 1; + spin_unlock(&scrub->os_lock); scrub_stop(scrub); up(&dev->od_otable_sem);