[sheepdog-users] Possible bug when adding participants to vdi

[Hitoshi Mitake]

Furkan, Vedvyas,

As Vedvyas agreed, Furukan's pointing seems to be valid. Vedvyas, could you
create a PR on our github repository?

Thanks,
Hitoshi

On Mon, Apr 25, 2016 at 4:53 PM, Vedvyas shanbhogue <vedvyas13686 at gmail.com>
wrote:

> Thanks . Yes, I made the fix without the +1 in my local checkout . Sorry
> about typo when sending the email.
> On Mon, Apr 25, 2016 at 1:17 AM <furkan at theshell.co.jp> wrote:
>
>> And one more reply (additional correction to my post),
>>
>> Now I understand the purpose of the proposed change a bit better.
>>
>> I believe it'd work OK like this;
>>
>> idx = entry->nr_participants;
>> // ...
>> entry->nr_participants++;
>>
>> The change here is to remove " + 1" from the first line of the proposed
>> change.
>> >> idx = entry->nr_participants + 1;
>>
>> Thanks,
>>
>> Furkan Mustafa
>>
>> On 2016-04-25 15:11, furkan at theshell.co.jp wrote:
>> > Hello,
>> >
>> > Sorry for butting in randomly, and sorry for top-posting.
>> > I'd like to point out a possible problem in the proposed change;
>> >
>> > Original Code:
>> >> idx = entry->nr_participants++;
>> >> ...
>> >
>> > Proposed Change:
>> >> idx = entry->nr_participants + 1;
>> >> ...
>> >> entry->nr_participants++;
>> >
>> > I didn't read the source but I believe that the original code here is
>> > making the assignment *before* increment on purpose.
>> >
>> > Because current number of elements in an array would be same number
>> > with
>> > the index of the next added item. and *after that*, number of items
>> > should
>> > be increased.
>> >
>> > Please correct me if I'm wrong.
>> >
>> > Thanks,
>> >
>> > Furkan Mustafa
>> >
>> > On 2016-04-25 14:09, Hitoshi Mitake wrote:
>> >> Hi Vedvyas, sorry for my late reply.
>> >>
>> >> On Sun, Apr 17, 2016 at 12:20 PM, Vedvyas shanbhogue
>> >> <vedvyas13686 at gmail.com> wrote:
>> >>
>> >>> This code increments the nr_participants but the participant state
>> >>> is not updated yet. The is_modified will now iterate over the
>> >>> updated nr_participants and can see bad state in participant_state
>> >>> array.
>> >>>
>> >>> idx = entry->nr_participants++;
>> >>> memcpy(&entry->participants[idx], owner, sizeof(*owner));
>> >>> entry->participants_state[idx] =
>> >>> is_modified(entry) ?
>> >>> SHARED_LOCK_STATE_INVALIDATED :
>> >>> SHARED_LOCK_STATE_SHARED;
>> >>>
>> >>> A fix may be:
>> >>>
>> >>> idx = entry->nr_participants+ 1;
>> >>> memcpy(&entry->participants[idx], owner, sizeof(*owner));
>> >>> entry->participants_state[idx] =
>> >>> is_modified(entry) ?
>> >>> SHARED_LOCK_STATE_INVALIDATED :
>> >>> SHARED_LOCK_STATE_SHARED;
>> >>> entry->nr_participants++;
>> >>>
>> >>> Please tell me if this would be a good fix.
>> >>>
>> >>> I was investigating a panic from is_modified where
>> >>> is_valid_shared_state finds an invalid shared state - "invalid
>> >>> shared state, two (or more) nodes are owning VDI..."
>> >>
>> >> Good catch, thanks a lot! Could you create a PR on github for your
>> >> change?
>> >>
>> >> Thanks,
>> >> Hitoshi
>>
>> --
>> sheepdog-users mailing lists
>> sheepdog-users at lists.wpkg.org
>> https://lists.wpkg.org/mailman/listinfo/sheepdog-users
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.wpkg.org/pipermail/sheepdog-users/attachments/20160426/516c48ba/attachment.html>