From: Ira Weiny <ira.weiny@intel.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-kernel@vger.kernel.org,
Alexander Viro <viro@zeniv.linux.org.uk>,
"Darrick J. Wong" <darrick.wong@oracle.com>,
Dan Williams <dan.j.williams@intel.com>,
Christoph Hellwig <hch@lst.de>,
"Theodore Y. Ts'o" <tytso@mit.edu>, Jan Kara <jack@suse.cz>,
linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH V4 01/13] fs/xfs: Remove unnecessary initialization of i_rwsem
Date: Thu, 27 Feb 2020 09:52:30 -0800 [thread overview]
Message-ID: <20200227175229.GA7072@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20200221012625.GT10776@dread.disaster.area>
On Fri, Feb 21, 2020 at 12:26:25PM +1100, Dave Chinner wrote:
> On Thu, Feb 20, 2020 at 04:41:22PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > xfs_reinit_inode() -> inode_init_always() already handles calling
> > init_rwsem(i_rwsem). Doing so again is unneeded.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>
> Except that this inode has been destroyed and freed by the VFS, and
> we are now recycling it back into the VFS before we actually
> physically freed it.
>
> Hence we have re-initialise the semaphore because the semaphore can
> contain internal state that is specific to it's new life cycle (e.g.
> the lockdep context) that will cause problems if we just assume that
> the inode is the same inode as it was before we recycled it back
> into the VFS caches.
>
> So, yes, we actually do need to re-initialise the rwsem here.
I'm fine dropping the patch...
But I'm not following how the xfs_reinit_inode() on line 422 does not take care
of this?
fs/xfs/xfs_icache.c:
422 error = xfs_reinit_inode(mp, inode);
423 if (error) {
424 bool wake;
425 /*
426 * Re-initializing the inode failed, and we are in deep
427 * trouble. Try to re-add it to the reclaim list.
428 */
429 rcu_read_lock();
430 spin_lock(&ip->i_flags_lock);
431 wake = !!__xfs_iflags_test(ip, XFS_INEW);
432 ip->i_flags &= ~(XFS_INEW | XFS_IRECLAIM);
433 if (wake)
434 wake_up_bit(&ip->i_flags, __XFS_INEW_BIT);
435 ASSERT(ip->i_flags & XFS_IRECLAIMABLE);
436 trace_xfs_iget_reclaim_fail(ip);
437 goto out_error;
438 }
439
440 spin_lock(&pag->pag_ici_lock);
441 spin_lock(&ip->i_flags_lock);
442
443 /*
444 * Clear the per-lifetime state in the inode as we are now
445 * effectively a new inode and need to return to the initial
446 * state before reuse occurs.
447 */
448 ip->i_flags &= ~XFS_IRECLAIM_RESET_FLAGS;
449 ip->i_flags |= XFS_INEW;
450 xfs_inode_clear_reclaim_tag(pag, ip->i_ino);
451 inode->i_state = I_NEW;
452 ip->i_sick = 0;
453 ip->i_checked = 0;
454
455 ASSERT(!rwsem_is_locked(&inode->i_rwsem));
456 init_rwsem(&inode->i_rwsem);
457
458 spin_unlock(&ip->i_flags_lock);
459 spin_unlock(&pag->pag_ici_lock);
Does this need to be done under one of these locks?
Ira
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
next prev parent reply other threads:[~2020-02-27 17:52 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-21 0:41 [PATCH V4 00/13] Enable per-file/per-directory DAX operations V4 ira.weiny
2020-02-21 0:41 ` [PATCH V4 01/13] fs/xfs: Remove unnecessary initialization of i_rwsem ira.weiny
2020-02-21 1:26 ` Dave Chinner
2020-02-27 17:52 ` Ira Weiny [this message]
2020-02-21 0:41 ` [PATCH V4 02/13] fs/xfs: Clarify lockdep dependency for xfs_isilocked() ira.weiny
2020-02-21 1:34 ` Dave Chinner
2020-02-21 23:00 ` Ira Weiny
2020-02-21 0:41 ` [PATCH V4 03/13] fs: Remove unneeded IS_DAX() check ira.weiny
2020-02-21 1:42 ` Dave Chinner
2020-02-21 23:04 ` Ira Weiny
2020-02-21 17:42 ` Christoph Hellwig
2020-02-21 0:41 ` [PATCH V4 04/13] fs/stat: Define DAX statx attribute ira.weiny
2020-02-21 0:41 ` [PATCH V4 05/13] fs/xfs: Isolate the physical DAX flag from enabled ira.weiny
2020-02-21 0:41 ` [PATCH V4 06/13] fs/xfs: Create function xfs_inode_enable_dax() ira.weiny
2020-02-22 0:28 ` Darrick J. Wong
2020-02-23 15:07 ` Ira Weiny
2020-02-21 0:41 ` [PATCH V4 07/13] fs: Add locking for a dynamic address space operations state ira.weiny
2020-02-21 17:44 ` Christoph Hellwig
2020-02-21 22:44 ` Dave Chinner
2020-02-21 23:26 ` Dan Williams
2020-02-24 17:56 ` Christoph Hellwig
2020-02-25 0:09 ` Dave Chinner
2020-02-25 17:36 ` Christoph Hellwig
2020-02-25 19:37 ` Jeff Moyer
2020-02-26 9:28 ` Jonathan Halliday
2020-02-26 11:31 ` Jan Kara
2020-02-26 11:56 ` Jonathan Halliday
2020-02-26 16:10 ` Ira Weiny
2020-02-26 16:46 ` Dan Williams
2020-02-26 17:20 ` Jan Kara
2020-02-26 17:54 ` Dan Williams
2020-02-25 21:03 ` Ira Weiny
2020-02-26 11:17 ` Jan Kara
2020-02-26 15:57 ` Ira Weiny
2020-02-22 0:33 ` Darrick J. Wong
2020-02-23 15:03 ` Ira Weiny
2020-02-24 12:10 ` kbuild test robot
2020-02-21 0:41 ` [PATCH V4 08/13] fs: Prevent DAX state change if file is mmap'ed ira.weiny
2020-02-21 0:41 ` [PATCH V4 09/13] fs/xfs: Add write aops lock to xfs layer ira.weiny
2020-02-22 0:31 ` Darrick J. Wong
2020-02-23 15:04 ` Ira Weiny
2020-02-24 0:34 ` Dave Chinner
2020-02-24 19:57 ` Ira Weiny
2020-02-24 22:32 ` Dave Chinner
2020-02-25 21:12 ` Ira Weiny
2020-02-25 22:59 ` Dave Chinner
2020-02-26 18:02 ` Ira Weiny
2020-02-21 0:41 ` [PATCH V4 10/13] fs/xfs: Clean up locking in dax invalidate ira.weiny
2020-02-21 17:45 ` Christoph Hellwig
2020-02-21 18:06 ` Ira Weiny
2020-02-21 0:41 ` [PATCH V4 11/13] fs/xfs: Allow toggle of effective DAX flag ira.weiny
2020-02-21 0:41 ` [PATCH V4 12/13] fs/xfs: Remove xfs_diflags_to_linux() ira.weiny
2020-02-21 0:41 ` [PATCH V4 13/13] Documentation/dax: Update Usage section ira.weiny
2020-02-26 22:48 ` [PATCH V4 00/13] Enable per-file/per-directory DAX operations V4 Jeff Moyer
2020-02-27 2:43 ` Ira Weiny
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200227175229.GA7072@iweiny-DESK2.sc.intel.com \
--to=ira.weiny@intel.com \
--cc=dan.j.williams@intel.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=hch@lst.de \
--cc=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.