| Message ID | 160083919333.1401135.1467785318303966214.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix some log stalling problems in defer ops | expand |
On Tue, Sep 22, 2020 at 10:33:13PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Separate the computation of the log push threshold and the push logic in > xlog_grant_push_ail. This enables higher level code to determine (for > example) that it is holding on to a logged intent item and the log is so > busy that it is more than 75% full. In that case, it would be desirable > to move the log item towards the head to release the tail, which we will > cover in the next patch. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_log.c | 41 +++++++++++++++++++++++++++++++---------- > fs/xfs/xfs_log.h | 2 ++ > 2 files changed, 33 insertions(+), 10 deletions(-) > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index ad0c69ee8947..62c9e0aaa7df 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1475,14 +1475,15 @@ xlog_commit_record( > } > > /* > - * Push on the buffer cache code if we ever use more than 75% of the on-disk > - * log space. This code pushes on the lsn which would supposedly free up > - * the 25% which we want to leave free. We may need to adopt a policy which > - * pushes on an lsn which is further along in the log once we reach the high > - * water mark. In this manner, we would be creating a low water mark. > + * Compute the LSN push target needed to push on the buffer cache code if we > + * ever use more than 75% of the on-disk log space. This code pushes on the > + * lsn which would supposedly free up the 25% which we want to leave free. We > + * may need to adopt a policy which pushes on an lsn which is further along in > + * the log once we reach the high water mark. In this manner, we would be > + * creating a low water mark. > */ Probably no need to duplicate so much of the original comment between the two functions. I'd just put something like "calculate the AIL push target based on the provided log reservation requirement ..." or otherwise just remove it. That aside, LGTM: Reviewed-by: Brian Foster <bfoster@redhat.com> > -STATIC void > -xlog_grant_push_ail( > +xfs_lsn_t > +xlog_grant_push_threshold( > struct xlog *log, > int need_bytes) > { > @@ -1508,7 +1509,7 @@ xlog_grant_push_ail( > free_threshold = max(free_threshold, (log->l_logBBsize >> 2)); > free_threshold = max(free_threshold, 256); > if (free_blocks >= free_threshold) > - return; > + return NULLCOMMITLSN; > > xlog_crack_atomic_lsn(&log->l_tail_lsn, &threshold_cycle, > &threshold_block); > @@ -1528,13 +1529,33 @@ xlog_grant_push_ail( > if (XFS_LSN_CMP(threshold_lsn, last_sync_lsn) > 0) > threshold_lsn = last_sync_lsn; > > + return threshold_lsn; > +} > + > +/* > + * Push on the buffer cache code if we ever use more than 75% of the on-disk > + * log space. This code pushes on the lsn which would supposedly free up > + * the 25% which we want to leave free. We may need to adopt a policy which > + * pushes on an lsn which is further along in the log once we reach the high > + * water mark. In this manner, we would be creating a low water mark. > + */ > +STATIC void > +xlog_grant_push_ail( > + struct xlog *log, > + int need_bytes) > +{ > + xfs_lsn_t threshold_lsn; > + > + threshold_lsn = xlog_grant_push_threshold(log, need_bytes); > + if (threshold_lsn == NULLCOMMITLSN || XLOG_FORCED_SHUTDOWN(log)) > + return; > + > /* > * Get the transaction layer to kick the dirty buffers out to > * disk asynchronously. No point in trying to do this if > * the filesystem is shutting down. > */ > - if (!XLOG_FORCED_SHUTDOWN(log)) > - xfs_ail_push(log->l_ailp, threshold_lsn); > + xfs_ail_push(log->l_ailp, threshold_lsn); > } > > /* > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > index 1412d6993f1e..58c3fcbec94a 100644 > --- a/fs/xfs/xfs_log.h > +++ b/fs/xfs/xfs_log.h > @@ -141,4 +141,6 @@ void xfs_log_quiesce(struct xfs_mount *mp); > bool xfs_log_check_lsn(struct xfs_mount *, xfs_lsn_t); > bool xfs_log_in_recovery(struct xfs_mount *); > > +xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); > + > #endif /* __XFS_LOG_H__ */ >
On Fri, Sep 25, 2020 at 07:15:11AM -0400, Brian Foster wrote: > On Tue, Sep 22, 2020 at 10:33:13PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Separate the computation of the log push threshold and the push logic in > > xlog_grant_push_ail. This enables higher level code to determine (for > > example) that it is holding on to a logged intent item and the log is so > > busy that it is more than 75% full. In that case, it would be desirable > > to move the log item towards the head to release the tail, which we will > > cover in the next patch. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_log.c | 41 +++++++++++++++++++++++++++++++---------- > > fs/xfs/xfs_log.h | 2 ++ > > 2 files changed, 33 insertions(+), 10 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > > index ad0c69ee8947..62c9e0aaa7df 100644 > > --- a/fs/xfs/xfs_log.c > > +++ b/fs/xfs/xfs_log.c > > @@ -1475,14 +1475,15 @@ xlog_commit_record( > > } > > > > /* > > - * Push on the buffer cache code if we ever use more than 75% of the on-disk > > - * log space. This code pushes on the lsn which would supposedly free up > > - * the 25% which we want to leave free. We may need to adopt a policy which > > - * pushes on an lsn which is further along in the log once we reach the high > > - * water mark. In this manner, we would be creating a low water mark. > > + * Compute the LSN push target needed to push on the buffer cache code if we > > + * ever use more than 75% of the on-disk log space. This code pushes on the > > + * lsn which would supposedly free up the 25% which we want to leave free. We > > + * may need to adopt a policy which pushes on an lsn which is further along in > > + * the log once we reach the high water mark. In this manner, we would be > > + * creating a low water mark. > > */ > > Probably no need to duplicate so much of the original comment between > the two functions. I'd just put something like "calculate the AIL push > target based on the provided log reservation requirement ..." or > otherwise just remove it. That aside, LGTM: Hmm, how about this for xlog_grant_push_threshold: /* * Compute the LSN that we'd need to push the log tail towards in order * to have (a) enough on-disk log space to log the number of bytes * specified, (b) at least 25% of the log space free, and (c) at least * 256 blocks free. If the log free space already meets all three * thresholds, this function returns NULLCOMMITLSN. */ and then this for xlog_grant_push_ail: /* * Push the tail of the log if we need to do so to maintain the free log * space thresholds set out by xlog_grant_push_threshold. We may need * to adopt a policy which pushes on an lsn which is further along in * the log once we reach the high water mark. In this manner, we would * be creating a low water mark. */ > Reviewed-by: Brian Foster <bfoster@redhat.com> Thanks for the review! --D > > -STATIC void > > -xlog_grant_push_ail( > > +xfs_lsn_t > > +xlog_grant_push_threshold( > > struct xlog *log, > > int need_bytes) > > { > > @@ -1508,7 +1509,7 @@ xlog_grant_push_ail( > > free_threshold = max(free_threshold, (log->l_logBBsize >> 2)); > > free_threshold = max(free_threshold, 256); > > if (free_blocks >= free_threshold) > > - return; > > + return NULLCOMMITLSN; > > > > xlog_crack_atomic_lsn(&log->l_tail_lsn, &threshold_cycle, > > &threshold_block); > > @@ -1528,13 +1529,33 @@ xlog_grant_push_ail( > > if (XFS_LSN_CMP(threshold_lsn, last_sync_lsn) > 0) > > threshold_lsn = last_sync_lsn; > > > > + return threshold_lsn; > > +} > > + > > +/* > > + * Push on the buffer cache code if we ever use more than 75% of the on-disk > > + * log space. This code pushes on the lsn which would supposedly free up > > + * the 25% which we want to leave free. We may need to adopt a policy which > > + * pushes on an lsn which is further along in the log once we reach the high > > + * water mark. In this manner, we would be creating a low water mark. > > + */ > > +STATIC void > > +xlog_grant_push_ail( > > + struct xlog *log, > > + int need_bytes) > > +{ > > + xfs_lsn_t threshold_lsn; > > + > > + threshold_lsn = xlog_grant_push_threshold(log, need_bytes); > > + if (threshold_lsn == NULLCOMMITLSN || XLOG_FORCED_SHUTDOWN(log)) > > + return; > > + > > /* > > * Get the transaction layer to kick the dirty buffers out to > > * disk asynchronously. No point in trying to do this if > > * the filesystem is shutting down. > > */ > > - if (!XLOG_FORCED_SHUTDOWN(log)) > > - xfs_ail_push(log->l_ailp, threshold_lsn); > > + xfs_ail_push(log->l_ailp, threshold_lsn); > > } > > > > /* > > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > > index 1412d6993f1e..58c3fcbec94a 100644 > > --- a/fs/xfs/xfs_log.h > > +++ b/fs/xfs/xfs_log.h > > @@ -141,4 +141,6 @@ void xfs_log_quiesce(struct xfs_mount *mp); > > bool xfs_log_check_lsn(struct xfs_mount *, xfs_lsn_t); > > bool xfs_log_in_recovery(struct xfs_mount *); > > > > +xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); > > + > > #endif /* __XFS_LOG_H__ */ > > >
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index ad0c69ee8947..62c9e0aaa7df 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1475,14 +1475,15 @@ xlog_commit_record( } /* - * Push on the buffer cache code if we ever use more than 75% of the on-disk - * log space. This code pushes on the lsn which would supposedly free up - * the 25% which we want to leave free. We may need to adopt a policy which - * pushes on an lsn which is further along in the log once we reach the high - * water mark. In this manner, we would be creating a low water mark. + * Compute the LSN push target needed to push on the buffer cache code if we + * ever use more than 75% of the on-disk log space. This code pushes on the + * lsn which would supposedly free up the 25% which we want to leave free. We + * may need to adopt a policy which pushes on an lsn which is further along in + * the log once we reach the high water mark. In this manner, we would be + * creating a low water mark. */ -STATIC void -xlog_grant_push_ail( +xfs_lsn_t +xlog_grant_push_threshold( struct xlog *log, int need_bytes) { @@ -1508,7 +1509,7 @@ xlog_grant_push_ail( free_threshold = max(free_threshold, (log->l_logBBsize >> 2)); free_threshold = max(free_threshold, 256); if (free_blocks >= free_threshold) - return; + return NULLCOMMITLSN; xlog_crack_atomic_lsn(&log->l_tail_lsn, &threshold_cycle, &threshold_block); @@ -1528,13 +1529,33 @@ xlog_grant_push_ail( if (XFS_LSN_CMP(threshold_lsn, last_sync_lsn) > 0) threshold_lsn = last_sync_lsn; + return threshold_lsn; +} + +/* + * Push on the buffer cache code if we ever use more than 75% of the on-disk + * log space. This code pushes on the lsn which would supposedly free up + * the 25% which we want to leave free. We may need to adopt a policy which + * pushes on an lsn which is further along in the log once we reach the high + * water mark. In this manner, we would be creating a low water mark. + */ +STATIC void +xlog_grant_push_ail( + struct xlog *log, + int need_bytes) +{ + xfs_lsn_t threshold_lsn; + + threshold_lsn = xlog_grant_push_threshold(log, need_bytes); + if (threshold_lsn == NULLCOMMITLSN || XLOG_FORCED_SHUTDOWN(log)) + return; + /* * Get the transaction layer to kick the dirty buffers out to * disk asynchronously. No point in trying to do this if * the filesystem is shutting down. */ - if (!XLOG_FORCED_SHUTDOWN(log)) - xfs_ail_push(log->l_ailp, threshold_lsn); + xfs_ail_push(log->l_ailp, threshold_lsn); } /* diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h index 1412d6993f1e..58c3fcbec94a 100644 --- a/fs/xfs/xfs_log.h +++ b/fs/xfs/xfs_log.h @@ -141,4 +141,6 @@ void xfs_log_quiesce(struct xfs_mount *mp); bool xfs_log_check_lsn(struct xfs_mount *, xfs_lsn_t); bool xfs_log_in_recovery(struct xfs_mount *); +xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); + #endif /* __XFS_LOG_H__ */