[Ocfs2-devel] [PATCH 1/1] ocfs2: fix ocfs2_find_slot repeats alloc same slot issue

[Junxiao Bi]

On 6/13/22 7:59 PM, heming.zhao at suse.com wrote:

> On 6/13/22 23:43, Junxiao Bi wrote:
>>
>>> 在 2022年6月13日，上午1:48，heming.zhao at suse.com 写道：
>>>
>>> On 6/13/22 16:21, Joseph Qi wrote:
>>>>> On 6/13/22 3:59 PM, heming.zhao at suse.com wrote:
>>>>> On 6/12/22 22:16, Joseph Qi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Why can't use local mount? I don't remember if we discuss about 
>>>>>> this.
>>>>>>
>>>>> Sorry, I can't follow your question.
>>>>> Do you mean why revert commit 912f655d78c5?
>>>>>
>>>>> or you are interest with the feature local mount?
>>>>> the local mount is created by mkfs.ocfs2, which can't be converted 
>>>>> to clustered.
>>>>> see mkfs.ocfs2(8) '-M' option.
>>>>>
>>>> What Junxiao's main concern is data corruption, so I'm afraid we 
>>>> have to
>>>> introduce an ondisk feature bit to prevent mixed nocluster and cluster
>>>> mount, similar to local mount.
>>>
>>> this patch defined two new variants/flags:
>>> #define OCFS2_SLOTMAP_CLUSTER   1
>>> #define OCFS2_SLOTMAP_NOCLUSTER 2
>>>
>>> (I expect) Under OCFS2_SLOTMAP_CLUSTER, for clustered mount mode and 
>>> for compatibility,
>>> anything doesn't need to be changed.
>>>
>>> OCFS2_SLOTMAP_NOCLUSTER will introduce a new value for slotmap area.
>>> this new value only take effect after a successfully nocluster mount.
>>> (pls fix me), existed kernel/user space code don't do any special 
>>> handle for
>>> noclustered mount mode in slotmap area. So the new value is also 
>>> compatibility.
>>>
>>> And the patch can also prevent mixed mount, the related code is in 
>>> ocfs2_find_slot().
>>> code logic:
>>> - noclustered mount condition: slotmap is empty or already mounted 
>>> with noclustered
>>> - clustered mount condition: slotmap is empty or already mounted 
>>> with clustered.
>>> - all other conditions will be denied.
>> Finding slot required reading slot map and then  update slot map. It 
>> is not atomic , you can’t prevent mixed mount until you have a 
>> cluster lock.
>
> Could I say your mentioned use case is invalid.
> I believe all (yes, *all*) the ocfs2 users use this fs under cluster 
> mode in
> their product env.
> The nocluster mount is only used on maintained env (eg. backup, fsck).
>
> We only concern two ways:
> 1. user forgets to unmount (eg crash) before using another mount mode.
> 2. when ocfs2 volume is working, which should deny volume is mounted 
> by another mode.
>    this may happened user mistakenly runs command or script.
>
> Base on above 1 & 2, Junxiao above mentioned use case only happens on 
> one scenario:
> the volume doesn't be mounted by any node, then user mistakenly mounts 
> volume
> with [no]cluster mode at same time. I believe this use case is 
> invalid. And I guess
> gfs2 may also has the same issue.

You missed the point, i never said that's the user case. The point is 
with your feature ocfs2 could be corrupted due to mistake done by 
customer, thinking about why fsck.ocfs2/mkfs.ocfs2 check that before 
changing anything.

You'd better make sure gfs2 had that issue, if so, i think you should 
raise a bug to them.

Thanks,

Junxiao.

> With this patch, ocfs2 has more sanity check ability than other fs, 
> eg: xfs, ext4.
> SUSE HA stack with corosync+pacemaker also supports running xfs/ext4 
> with A/P mode.
> The xfs/ext4 never have detecting mount status code, Junxiao mentioned 
> mixed mount
> can also happens on these fs. How do xfs/ext4/HA maintainers handle 
> it? Under these
> fs mounting behavior, these fields maintainers also treat the mixed 
> mount as
> invalid use case and ignore handle it.
>
> /Heming
>
>>>
>>>> Another scenario is journal replay after crash.
>>>
>>> this patch set a rule:
>>> If last mount didn't do umount, (eg: crash happened), the next mount 
>>> MUST be same mount type.
>>> (please also check above lines of 'code logic'.)
>>>
>>> In my view, this rule is enough to handle crash scenario.
>>> So my patch should be polished in somewhere, but it is workable.
>>>
>>> Thanks,
>>> Heming
>