Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Al Viro <viro@ZenIV.linux.org.uk>]

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.