| Message ID | 1471816273-28940-4-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Sun, Aug 21, 2016 at 11:51:12PM +0200, Christoph Hellwig wrote: > For long growing file writes we will usually already have the eofblocks > tag set when adding more speculative preallocations. Add a flag in the > inode to allow us to skip the the fairly expensive AG-wide spinlocks > and multiple radix tree operations in that case. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_icache.c | 14 ++++++++++++++ > fs/xfs/xfs_inode.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index fb39a66..65b2e3f 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -1414,6 +1414,16 @@ xfs_inode_set_eofblocks_tag( > struct xfs_perag *pag; > int tagged; > > + /* > + * Don't bother locking the AG and looking up in the radix trees > + * if we already know that we have the tag set. > + */ > + if (ip->i_flags & XFS_IEOFBLOCKS) > + return; > + spin_lock(&ip->i_flags_lock); > + ip->i_flags |= XFS_IEOFBLOCKS; > + spin_unlock(&ip->i_flags_lock); > + I'm guessing the lockless check is intentional, but is that really necessary? E.g., it doesn't seem like using ->i_flags_lock unconditionally should affect performance in the way the AG lock or radix tree work does, particularly since we're already holding IOLOCK_EXCL in the current implementation. I could be wrong, but FWIW, we do already have xfs_iflags_test_and_set() sitting around as well... Brian > pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino)); > spin_lock(&pag->pag_ici_lock); > trace_xfs_inode_set_eofblocks_tag(ip); > @@ -1449,6 +1459,10 @@ xfs_inode_clear_eofblocks_tag( > struct xfs_mount *mp = ip->i_mount; > struct xfs_perag *pag; > > + spin_lock(&ip->i_flags_lock); > + ip->i_flags &= ~XFS_IEOFBLOCKS; > + spin_unlock(&ip->i_flags_lock); > + > pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino)); > spin_lock(&pag->pag_ici_lock); > trace_xfs_inode_clear_eofblocks_tag(ip); > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index e1a411e..8f30d25 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -216,6 +216,7 @@ xfs_get_initial_prid(struct xfs_inode *dp) > #define __XFS_IPINNED_BIT 8 /* wakeup key for zero pin count */ > #define XFS_IPINNED (1 << __XFS_IPINNED_BIT) > #define XFS_IDONTCACHE (1 << 9) /* don't cache the inode long term */ > +#define XFS_IEOFBLOCKS (1 << 10)/* has the preallocblocks tag set */ > > /* > * Per-lifetime flags need to be reset when re-using a reclaimable inode during > -- > 2.1.4 > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs
On Thu, Aug 25, 2016 at 08:38:09AM -0400, Brian Foster wrote: > I'm guessing the lockless check is intentional, but is that really > necessary? E.g., it doesn't seem like using ->i_flags_lock > unconditionally should affect performance in the way the AG lock or > radix tree work does, particularly since we're already holding > IOLOCK_EXCL in the current implementation. I could be wrong, but FWIW, > we do already have xfs_iflags_test_and_set() sitting around as well... I don't think taking it should be too bad, but given the ops ordering it also seems entirely pointless to even take it.
On Fri, Aug 26, 2016 at 04:26:16PM +0200, Christoph Hellwig wrote: > On Thu, Aug 25, 2016 at 08:38:09AM -0400, Brian Foster wrote: > > I'm guessing the lockless check is intentional, but is that really > > necessary? E.g., it doesn't seem like using ->i_flags_lock > > unconditionally should affect performance in the way the AG lock or > > radix tree work does, particularly since we're already holding > > IOLOCK_EXCL in the current implementation. I could be wrong, but FWIW, > > we do already have xfs_iflags_test_and_set() sitting around as well... > > I don't think taking it should be too bad, but given the ops ordering > it also seems entirely pointless to even take it. > Then why are we taking it? I assumed it at least served as a memory barrier... Brian > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs
On Fri, Aug 26, 2016 at 12:02:09PM -0400, Brian Foster wrote: > > I don't think taking it should be too bad, but given the ops ordering > > it also seems entirely pointless to even take it. > > > > Then why are we taking it? I assumed it at least served as a memory > barrier... I meant to take it for that early check, not in general. I guess this is another hint we should try to look into using proper atomic bitops here..
On Tue, Aug 30, 2016 at 04:40:06PM +0200, Christoph Hellwig wrote: > On Fri, Aug 26, 2016 at 12:02:09PM -0400, Brian Foster wrote: > > > I don't think taking it should be too bad, but given the ops ordering > > > it also seems entirely pointless to even take it. > > > > > > > Then why are we taking it? I assumed it at least served as a memory > > barrier... > > I meant to take it for that early check, not in general. > > I guess this is another hint we should try to look into using proper > atomic bitops here.. I think we've looked at that in the past, but there were cases where we have to do things atomically with setting/clearing the flags and that required the spinlock to protect the flag modifications as well. IIRC there are also cases where we have to check/set multiple flags at once, which we cannot do with atomic bit ops. Perhaps the code has changed enough that there isn't a problem anymore, but I don't think that is the case... Cheers, Dave.
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index fb39a66..65b2e3f 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -1414,6 +1414,16 @@ xfs_inode_set_eofblocks_tag( struct xfs_perag *pag; int tagged; + /* + * Don't bother locking the AG and looking up in the radix trees + * if we already know that we have the tag set. + */ + if (ip->i_flags & XFS_IEOFBLOCKS) + return; + spin_lock(&ip->i_flags_lock); + ip->i_flags |= XFS_IEOFBLOCKS; + spin_unlock(&ip->i_flags_lock); + pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino)); spin_lock(&pag->pag_ici_lock); trace_xfs_inode_set_eofblocks_tag(ip); @@ -1449,6 +1459,10 @@ xfs_inode_clear_eofblocks_tag( struct xfs_mount *mp = ip->i_mount; struct xfs_perag *pag; + spin_lock(&ip->i_flags_lock); + ip->i_flags &= ~XFS_IEOFBLOCKS; + spin_unlock(&ip->i_flags_lock); + pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, ip->i_ino)); spin_lock(&pag->pag_ici_lock); trace_xfs_inode_clear_eofblocks_tag(ip); diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h index e1a411e..8f30d25 100644 --- a/fs/xfs/xfs_inode.h +++ b/fs/xfs/xfs_inode.h @@ -216,6 +216,7 @@ xfs_get_initial_prid(struct xfs_inode *dp) #define __XFS_IPINNED_BIT 8 /* wakeup key for zero pin count */ #define XFS_IPINNED (1 << __XFS_IPINNED_BIT) #define XFS_IDONTCACHE (1 << 9) /* don't cache the inode long term */ +#define XFS_IEOFBLOCKS (1 << 10)/* has the preallocblocks tag set */ /* * Per-lifetime flags need to be reset when re-using a reclaimable inode during
For long growing file writes we will usually already have the eofblocks tag set when adding more speculative preallocations. Add a flag in the inode to allow us to skip the the fairly expensive AG-wide spinlocks and multiple radix tree operations in that case. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_icache.c | 14 ++++++++++++++ fs/xfs/xfs_inode.h | 1 + 2 files changed, 15 insertions(+)