| Message ID | 20190517073119.30178-6-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [01/20] xfs: fix a trivial comment typo in the xfs_trans_committed_bulk | expand |
On Fri, May 17, 2019 at 09:31:04AM +0200, Christoph Hellwig wrote: > If we want to push the log to make progress on the items we'd need to > return XFS_ITEM_PINNED instead of XFS_ITEM_LOCKED. Removing the > method will do exactly that. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_dquot_item.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c > index 486eea151fdb..a61a8a770d7f 100644 > --- a/fs/xfs/xfs_dquot_item.c > +++ b/fs/xfs/xfs_dquot_item.c > @@ -288,18 +288,6 @@ xfs_qm_qoff_logitem_format( > xlog_finish_iovec(lv, vecp, sizeof(struct xfs_qoff_logitem)); > } > > -/* > - * There isn't much you can do to push a quotaoff item. It is simply > - * stuck waiting for the log to be flushed to disk. > - */ > -STATIC uint > -xfs_qm_qoff_logitem_push( > - struct xfs_log_item *lip, > - struct list_head *buffer_list) > -{ > - return XFS_ITEM_LOCKED; > -} > - Hmm, this one is a bit interesting because it's a potential change in behavior and I'm not sure the comment above accurately reflects the situation. In xfs_qm_scall_quotaoff(), we log the first quotaoff item and commit it synchronously. I believe this means it immediately goes into the AIL. Then we have to iterate inodes to drop all dquot references and purge the dquot cache, which can do I/O by writing back dquot bufs before we eventually log the quotaoff_end item. All in all this can take a bit of time (and we have test scenarios that reproduce quotaoff log deadlocks already). I think this change can cause AIL processing concurrent to a quotaoff in progress to potentially force the log on every pass. I would not expect that to have a positive effect because a log force doesn't actually help the quotaoff progress until the quotaoff_end is committed, and that already occurs synchronously as well. I don't think it's wise to change behavior here, at least not without some testing and analysis around how this impacts those already somewhat flakey quota off operations. Brian > STATIC xfs_lsn_t > xfs_qm_qoffend_logitem_committed( > struct xfs_log_item *lip, > @@ -327,7 +315,6 @@ static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = { > .iop_size = xfs_qm_qoff_logitem_size, > .iop_format = xfs_qm_qoff_logitem_format, > .iop_committed = xfs_qm_qoffend_logitem_committed, > - .iop_push = xfs_qm_qoff_logitem_push, > }; > > /* > @@ -336,7 +323,6 @@ static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = { > static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = { > .iop_size = xfs_qm_qoff_logitem_size, > .iop_format = xfs_qm_qoff_logitem_format, > - .iop_push = xfs_qm_qoff_logitem_push, > }; > > /* > -- > 2.20.1 >
On Fri, May 17, 2019 at 10:08:42AM -0400, Brian Foster wrote: > Hmm, this one is a bit interesting because it's a potential change in > behavior and I'm not sure the comment above accurately reflects the > situation. In xfs_qm_scall_quotaoff(), we log the first quotaoff item > and commit it synchronously. I believe this means it immediately goes > into the AIL. Then we have to iterate inodes to drop all dquot > references and purge the dquot cache, which can do I/O by writing back > dquot bufs before we eventually log the quotaoff_end item. All in all > this can take a bit of time (and we have test scenarios that reproduce > quotaoff log deadlocks already). > > I think this change can cause AIL processing concurrent to a quotaoff in > progress to potentially force the log on every pass. I would not expect > that to have a positive effect because a log force doesn't actually help > the quotaoff progress until the quotaoff_end is committed, and that > already occurs synchronously as well. I don't think it's wise to change > behavior here, at least not without some testing and analysis around how > this impacts those already somewhat flakey quota off operations. True, the log force probably doesn't help. I'll drop this for now, the whole quotaoff logging scheme looks pretty dodgy to me to start with, so it will need some more attention in the future.
diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c index 486eea151fdb..a61a8a770d7f 100644 --- a/fs/xfs/xfs_dquot_item.c +++ b/fs/xfs/xfs_dquot_item.c @@ -288,18 +288,6 @@ xfs_qm_qoff_logitem_format( xlog_finish_iovec(lv, vecp, sizeof(struct xfs_qoff_logitem)); } -/* - * There isn't much you can do to push a quotaoff item. It is simply - * stuck waiting for the log to be flushed to disk. - */ -STATIC uint -xfs_qm_qoff_logitem_push( - struct xfs_log_item *lip, - struct list_head *buffer_list) -{ - return XFS_ITEM_LOCKED; -} - STATIC xfs_lsn_t xfs_qm_qoffend_logitem_committed( struct xfs_log_item *lip, @@ -327,7 +315,6 @@ static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = { .iop_size = xfs_qm_qoff_logitem_size, .iop_format = xfs_qm_qoff_logitem_format, .iop_committed = xfs_qm_qoffend_logitem_committed, - .iop_push = xfs_qm_qoff_logitem_push, }; /* @@ -336,7 +323,6 @@ static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = { static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = { .iop_size = xfs_qm_qoff_logitem_size, .iop_format = xfs_qm_qoff_logitem_format, - .iop_push = xfs_qm_qoff_logitem_push, }; /*
If we want to push the log to make progress on the items we'd need to return XFS_ITEM_PINNED instead of XFS_ITEM_LOCKED. Removing the method will do exactly that. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_dquot_item.c | 14 -------------- 1 file changed, 14 deletions(-)