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;
IronPort-SDR: JmhUZxUexb+kDP8QNLAvCaCTel6sZvt91vBGx4C+NlSYwB0xnGr6KDvapmV45q7inIzCMq7j7F
 BDaW/qnVtqMnHmowB1tievXgMVi4HIStNZOpzWM3aSpAVLk1PIBvUWfq4e1SRDAbeQV0IqVAQ2
 VkzyLak7dbaFIB/6HC8a/nps24LKvKdg8IjW3OQWyD5UNUA3E4uy9dj3l6V1s87u9J5MU8I6xa
 B+p6z9GtvrMkReLyrMfeHPhNVRcw//E5AWH/llXCtL4MjgCbA6U4xhxtsqfvNFXErCYulifuRE
 O10=
From:   Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
To:     Bo YU <tsu.yubo@gmail.com>
CC:     "clm@fb.com" <clm@fb.com>,
        "josef@toxicpanda.com" <josef@toxicpanda.com>,
        "sterba@suse.com" <sterba@suse.com>,
        "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH -next] fs/btrfs: Fix unlocking in btrfs_ref_tree_mod
Thread-Topic: [PATCH -next] fs/btrfs: Fix unlocking in btrfs_ref_tree_mod
Thread-Index: AQHWKl8K6GclIOarbkSiVL0aew+onA==
Date:   Fri, 15 May 2020 09:36:49 +0000
Message-ID: <SN4PR0401MB35989B816CB1DA4BCB75B1FC9BBD0@SN4PR0401MB3598.namprd04.prod.outlook.com>
References: <20200515021731.cb5y557wsxf66fo3@debian.debian-2>
 <SN4PR0401MB35985CFC199D20362EBFD8E09BBD0@SN4PR0401MB3598.namprd04.prod.outlook.com>
 <CAKq8=3KyewqQLdo-GjERuOfKe5ZrmQ+bRPfFRWiyZkjdEVvSeA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ddcb7ed0-637d-4043-5011-08d7f8b37e07
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 May 2020 09:36:49.2597
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: pl98wqoCwqYWCsWJSi7Nwbm5/EWyCX1Ygew+eczcFmE2GGppL3lrm8F89CPdVogAtc+0Lmp01kdLiTFEzMvaCkUY4qPzSOnZhoubXikeo4E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR0401MB3695
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 15/05/2020 11:24, Bo YU wrote:=0A=
> Hi,=0A=
> On Fri, May 15, 2020 at 5:03 PM Johannes Thumshirn=0A=
> <Johannes.Thumshirn@wdc.com> wrote:=0A=
>>=0A=
>> On 15/05/2020 04:17, Bo YU wrote:=0A=
>>> It adds spin_lock() in add_block_entry() but out path does not unlock=
=0A=
>>> it.=0A=
>>=0A=
>> Which call path doesn't unlock it? There is an out_unlock label with a=
=0A=
>> spin_unlock() right above your insert. So either coverity messed somethi=
ng=0A=
>> up or the call path that needs the unlock has to jump to out_unlock inst=
ead=0A=
>> of out.=0A=
> This is out label without unlocking it. It will be offered spin_lock=0A=
> in add_block_entry()=0A=
> for be. But here I was worried about that unlock it in if() whether it=0A=
> is right or not.=0A=
> =0A=
=0A=
No add_block_entry() returns with the ref_verify_lock held on success only:=
=0A=
static struct block_entry *add_block_entry(struct btrfs_fs_info *fs_info,=
=0A=
                                           u64 bytenr, u64 len,            =
                                                                           =
                               =0A=
                                           u64 root_objectid)=0A=
{               =0A=
        struct block_entry *be =3D NULL, *exist;                           =
                                                                           =
                                 =0A=
        struct root_entry *re =3D NULL;                                    =
                                                                           =
                                 =0A=
                        =0A=
        re =3D kzalloc(sizeof(struct root_entry), GFP_KERNEL);             =
                                                                           =
                                 =0A=
        be =3D kzalloc(sizeof(struct block_entry), GFP_KERNEL);            =
                                                                           =
                                 =0A=
        if (!be || !re) {=0A=
                kfree(re);=0A=
                kfree(be);=0A=
                return ERR_PTR(-ENOMEM);                                   =
                                                                           =
                          =0A=
        }       =0A=
        be->bytenr =3D bytenr;=0A=
        be->len =3D len;  =0A=
                        =0A=
        re->root_objectid =3D root_objectid;=0A=
        re->num_refs =3D 0;=0A=
                        =0A=
        spin_lock(&fs_info->ref_verify_lock);                          =0A=
[...]=0A=
=0A=
=0A=
While the code caller checks for an error:=0A=
=0A=
if (action =3D=3D BTRFS_ADD_DELAYED_EXTENT) {=0A=
                /*=0A=
                 * For subvol_create we'll just pass in whatever the parent=
 root=0A=
                 * is and the new root objectid, so let's not treat the pas=
sed=0A=
                 * in root as if it really has a ref for this bytenr.=0A=
                 */=0A=
                be =3D add_block_entry(fs_info, bytenr, num_bytes, ref_root=
);=0A=
                if (IS_ERR(be)) {=0A=
                        kfree(ref);=0A=
                        kfree(ra);=0A=
                        ret =3D PTR_ERR(be);=0A=
                        goto out;=0A=
                }=0A=
=0A=
So if add_block_entry returns -ENOMEM it didn't take the lock and thus no u=
nlock=0A=
is needed.=0A=
=0A=
Or did I miss something?=0A=