From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-pf1-f194.google.com ([209.85.210.194]:34261 "EHLO
        mail-pf1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727053AbeJQJym (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 17 Oct 2018 05:54:42 -0400
Received: by mail-pf1-f194.google.com with SMTP id f78-v6so6610003pfe.1
        for <linux-fsdevel@vger.kernel.org>; Tue, 16 Oct 2018 19:01:24 -0700 (PDT)
Message-ID: <1539741681.3153.5.camel@slavad-ubuntu-14.04>
Subject: Re: [PATCH 1/2] hfsplus: update timestamps on truncate()
From: Viacheslav Dubeyko <slava@dubeyko.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: "Ernesto A." =?ISO-8859-1?Q?Fern=E1ndez?=
        <ernesto.mnd.fernandez@gmail.com>, linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 16 Oct 2018 19:01:21 -0700
In-Reply-To: <20181015212416.GY32577@ZenIV.linux.org.uk>
References: <9beb0913eea37288599e8e1b7cec8768fb52d1b8.1539316825.git.ernesto.mnd.fernandez@gmail.com>
         <1539381441.3203.6.camel@slavad-ubuntu-14.04>
         <20181013024256.GI32577@ZenIV.linux.org.uk>
         <1539637321.3133.2.camel@slavad-ubuntu-14.04>
         <20181015212416.GY32577@ZenIV.linux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Mon, 2018-10-15 at 22:24 +0100, Al Viro wrote:
> On Mon, Oct 15, 2018 at 02:02:01PM -0700, Viacheslav Dubeyko wrote:
> > > 1) unlink() finds the victim and locks it
> > > 
> > > 2) in hfsplus_file_release():
> > >         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
> > > got to 0
> > >                 inode_lock(inode);
> > > block waiting for unlink
> > > 
> > > 3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
> > > we do
> > >         atomic_inc(&HFSPLUS_I(inode)->opencnt);
> > > and now opencnt is 1.
> > > 
> > > 4) on the unlink side:
> > >         if (inode->i_ino == cnid &&
> > >             atomic_read(&HFSPLUS_I(inode)->opencnt)) {
> > >                 str.name = name;
> > >                 str.len = sprintf(name, "temp%lu", inode->i_ino);
> > >                 res = hfsplus_rename_cat(inode->i_ino,
> > >                                          dir, &dentry->d_name,
> > >                                          sbi->hidden_dir, &str);
> > >                 if (!res) {
> > >                         inode->i_flags |= S_DEAD;
> > >                         drop_nlink(inode);
> > >                 }
> > >                 goto out;
> > >         }
> > > nlink is zero now, the sucker got renamed and marked S_DEAD
> > > 
> > > 5) ->release() finally got through inode_lock() and
> > >                 hfsplus_file_truncate(inode);
> > >                 if (inode->i_flags & S_DEAD) {
> > >                         hfsplus_delete_cat(inode->i_ino,
> > >                                            HFSPLUS_SB(sb)->hidden_dir, NULL);
> > >                         hfsplus_delete_inode(inode);
> > >                 }
> > >                 inode_unlock(inode);
> > > ... and now we have killed everything we used to have associated with that
> > > inode on disk.  While it's still open.  What's to stop CNID to be reused,
> > > etc. and what's to preserve sanity in that situation?
> > > 
> > > What am I missing there?
> > 
> > OK. If you believe that the patch is in the bad shape then what's your
> > suggestion for improving the patch?
> 
> Tha patch is OK; the problem, AFAICS, is neither introduced nor fixed by it.
> I might be wrong regarding the locking problem I described, but it does
> appear to be real and I'd like somebody more familiar with HFS+ to comment
> on it.

Yes, it looks like a mess. Let me take a deeper look in the code.

Thanks,
Vyacheslav Dubeyko.