Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx> writes:
> On 12/06/2016 03:30 PM, Jes Sorensen wrote:
>> Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx> writes:
>>> On 12/05/2016 10:50 PM, Jes Sorensen wrote:
>>>> Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx> writes:
>>>>> This extends the --rwh-policy parameter to work also in Misc mode. Using
>>>>> it changes the currently active RWH policy in the kernel driver and
>>>>> updates the metadata to make this change permanent. Updating metadata is
>>>>> not yet implemented for super1, so this is limited to IMSM for now.
>>>>>
>>>>> Signed-off-by: Artur Paszkiewicz <artur.paszkiewicz@xxxxxxxxx>
>>>>
>>>> Hi Artur,
>>>>
>>>> It looked good all the way up until 6/7, but there is a nit here:
>>>>
>>>>> ---
>>>>> Manage.c | 79
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> mdadm.c | 9 +++++++
>>>>> mdadm.h | 1 +
>>>>> super-intel.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>>> 4 files changed, 153 insertions(+), 1 deletion(-)
>>>> [snip]
>>>>> diff --git a/super-intel.c b/super-intel.c
>>>>> index e524ef0..3b40429 100644
>>>>> --- a/super-intel.c
>>>>> +++ b/super-intel.c
>>>>> @@ -448,6 +448,7 @@ enum imsm_update_type {
>>>>> update_general_migration_checkpoint,
>>>>> update_size_change,
>>>>> update_prealloc_badblocks_mem,
>>>>> + update_rwh_policy,
>>>>> };
>>>>>
>>>>> struct imsm_update_activate_spare {
>>>>> @@ -540,6 +541,12 @@ struct imsm_update_prealloc_bb_mem {
>>>>> enum imsm_update_type type;
>>>>> };
>>>>>
>>>>> +struct imsm_update_rwh_policy {
>>>>> + enum imsm_update_type type;
>>>>> + int new_policy;
>>>>> + int dev_idx;
>>>>> +};
>>>>> +
>>>>> static const char *_sys_dev_type[] = {
>>>>> [SYS_DEV_UNKNOWN] = "Unknown",
>>>>> [SYS_DEV_SAS] = "SAS",
>>>>> @@ -3175,7 +3182,6 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
>>>>> info->custom_array_size <<= 32;
>>>>> info->custom_array_size |= __le32_to_cpu(dev->size_low);
>>>>> info->recovery_blocked = imsm_reshape_blocks_arrays_changes(st->sb);
>>>>> - info->journal_clean = dev->rwh_policy;
>>>>>
>>>>> if (is_gen_migration(dev)) {
>>>>> info->reshape_active = 1;
>>>>> @@ -3347,6 +3353,8 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
>>>>> info->rwh_policy = RWH_POLICY_PPL;
>>>>> else
>>>>> info->rwh_policy = RWH_POLICY_UNKNOWN;
>>>>> +
>>>>> + info->journal_clean = info->rwh_policy == RWH_POLICY_PPL;
>>>>> }
>>>>> }
>>>>
>>>> This part doesn't make sense, first you set info->rwh_policy based on
>>>> sb->feature_map to RWH_POLICY_PPL or RWH_POLICY_UNKNOWN and then right
>>>> after you hard set it to RWH_POLICY_PPL.
>>>>
>>>> In general I really would prefer not to see any of those double
>>>> assignments if it can be avoided.
>>>
>>> This isn't a double assignment, there is a '==' there. I'm setting
>>> info->journal_clean to true only if the policy is PPL. I'm not sure how
>>> this change ended up in this patch, it was supposed to go to 5/7. I must
>>> have overlooked it.
>>
>> Argh you're right, code obfuscation at it's finest - if this is meant to
>> be in 5/7 do you want to respin the two?
>>
>> In addition why not put the info->journal_clean assignments up together
>> with the info->rhw_policy assignments? Would make it a lot easier to
>> read without making my mistake :)
>
> Sure, I can change that :) If you agree with the rest I'll just resend
> those two patches.
Yes, that would be perfect.
Thanks,
Jes
--
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
