Re: [PATCH 5/6] Check write journal in incremental

[Neil Brown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3500 bytes --]

Dan Williams <dan.j.williams@intel.com> writes:

> On Fri, Aug 28, 2015 at 4:27 PM, Song Liu <songliubraving@fb.com> wrote:
>> If journal device is missing, do not start the array, and shows:
>>
>> ./mdadm -I /dev/sdf
>> mdadm: journal device is missing, not safe to start yet.
>>
>> The array will be started when the journal device is attached with -I
>>
>> ./mdadm -I /dev/sdb1
>> mdadm: /dev/sdb1 attached to /dev/md/0_0, which has been started.
>>
>> To force start without journal device:
>>
>> ./mdadm -I /dev/sdf --run
>> mdadm: Trying to run with missing journal device
>> mdadm: /dev/sdf attached to /dev/md/0_0, which has been started.
>>
>> Signed-off-by: Shaohua Li <shli@fb.com>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>>  Incremental.c | 31 +++++++++++++++++++++++++++----
>>  1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/Incremental.c b/Incremental.c
>> index 304cc6d..74905e3 100644
>> --- a/Incremental.c
>> +++ b/Incremental.c
>> @@ -35,7 +35,7 @@
>>
>>  static int count_active(struct supertype *st, struct mdinfo *sra,
>>                         int mdfd, char **availp,
>> -                       struct mdinfo *info);
>> +                       struct mdinfo *info, int *journal_device_missing);
>>  static void find_reject(int mdfd, struct supertype *st, struct mdinfo *sra,
>>                         int number, __u64 events, int verbose,
>>                         char *array_name);
>> @@ -104,6 +104,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
>>         struct map_ent target_array;
>>         int have_target;
>>         char *devname = devlist->devname;
>> +       int journal_device_missing = 0;
>>
>>         struct createinfo *ci = conf_get_create_info();
>>
>> @@ -518,7 +519,7 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
>>         sysfs_free(sra);
>>         sra = sysfs_read(mdfd, NULL, (GET_DEVS | GET_STATE |
>>                                     GET_OFFSET | GET_SIZE));
>> -       active_disks = count_active(st, sra, mdfd, &avail, &info);
>> +       active_disks = count_active(st, sra, mdfd, &avail, &info, &journal_device_missing);
>>         if (enough(info.array.level, info.array.raid_disks,
>>                    info.array.layout, info.array.state & 1,
>>                    avail) == 0) {
>> @@ -548,10 +549,12 @@ int Incremental(struct mddev_dev *devlist, struct context *c,
>>         }
>>
>>         map_unlock(&map);
>> -       if (c->runstop > 0 || active_disks >= info.array.working_disks) {
>> +       if (c->runstop > 0 || (!journal_device_missing && active_disks >= info.array.working_disks)) {
>
> A minor comment, and I'd defer to Neil's opinion, but I think this is
> asking for mdu_array_info_t to grow a "journal_disks" attribute.

We definitely don't want to change mdu_array_info_t - that is part of
the kernel api.
But adding a 'journal_disks' field to 'struct mdinfo' might make sense.

Similarly, now that I look at it, the 'require_journal' method doesn't
look like such a good idea.
->getinfo_super should set some flag in 'struct mdinfo' if a journal is
required.

The Incremental() function might still have a local var called
journal_device_missing, but it wouldn't pass it to count_active().
count_active would just set ->journal_disks and then Incrmental would do
something like:

journal_device_missing = info.journal_needed && info.journal_disks == 0;

Song: can you see if making some changes like that works out?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]