Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 4.19 114/131] ocfs2: avoid inode removal while nfsd is
 accessing it
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        Pavel Machek <pavel@denx.de>
Cc:     Sasha Levin <sashal@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        stable <stable@vger.kernel.org>,
        Joseph Qi <joseph.qi@linux.alibaba.com>,
        Changwei Ge <gechangwei@live.cn>, Gang He <ghe@suse.com>,
        Joel Becker <jlbec@evilplan.org>,
        Jun Piao <piaojun@huawei.com>, Mark Fasheh <mark@fasheh.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <20200629153502.2494656-1-sashal@kernel.org>
 <20200629153502.2494656-115-sashal@kernel.org> <20200702211717.GC5787@amd>
 <CAHk-=wj1VVZoNvtVYL9wCPVjBHwxhCXd4TfKbY0-OsG4nGyf2w@mail.gmail.com>
From:   Junxiao Bi <junxiao.bi@oracle.com>
Message-ID: <c2a0af77-2714-202b-43d9-7100daeb80b1@oracle.com>
Date:   Sat, 4 Jul 2020 18:49:20 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAHk-=wj1VVZoNvtVYL9wCPVjBHwxhCXd4TfKbY0-OsG4nGyf2w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 7/2/20 3:24 PM, Linus Torvalds wrote:

> On Thu, Jul 2, 2020 at 2:17 PM Pavel Machek <pavel@denx.de> wrote:
>>
>>> commit 4cd9973f9ff69e37dd0ba2bd6e6423f8179c329a upstream.
>>>
>>> Patch series "ocfs2: fix nfsd over ocfs2 issues", v2.
>> This causes locking imbalance:
> This sems to be true upstream too.
>
>> When ocfs2_nfs_sync_lock() returns error, caller can not know if the
>> lock was taken or not.
> Right you are.
>
> And your patch looks sane:
>
>> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
>> index c141b06811a6..8149fb6f1f0d 100644
>> --- a/fs/ocfs2/dlmglue.c
>> +++ b/fs/ocfs2/dlmglue.c
>> @@ -2867,9 +2867,15 @@ int ocfs2_nfs_sync_lock(struct ocfs2_super *osb, int ex)
>>
>>          status = ocfs2_cluster_lock(osb, lockres, ex ? LKM_EXMODE : LKM_PRMODE,
>>                                      0, 0);
>> -       if (status < 0)
>> +       if (status < 0) {
>>                  mlog(ML_ERROR, "lock on nfs sync lock failed %d\n", status);
>>
>> +               if (ex)
>> +                       up_write(&osb->nfs_sync_rwlock);
>> +               else
>> +                       up_read(&osb->nfs_sync_rwlock);
>> +       }
>> +
>>          return status;
>>   }
> although the whole thing looks messy.
>
> If the issue is a lifetime thing (like that commit says), the proper
> model isn't a lock, but a refcount.
>
> Oh well. Junxiao?

There is a block number embedded in nfs file handle, to verify it's an 
inode, need acquire this nfs_sync_lock global lock to avoid any inode 
removed from local node and other nodes in the cluster, before this 
verify done, seemed no way to use a refcount.

Thanks,

Junxiao.

>
>                 Linus