| Message ID | 20210106174127.805660-3-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rework log quiesce to cover the log | expand |
On 1/6/21 10:41 AM, Brian Foster wrote: > The log covering helper checks whether the filesystem is writable to > determine whether to cover the log. The helper is currently only > called from the background log worker. In preparation to reuse the > helper from freezing contexts, lift the check into xfs_log_worker(). > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks straight forward Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/xfs_log.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index b445e63cbc3c..4137ed007111 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1050,13 +1050,11 @@ xfs_log_space_wake( > * can't start trying to idle the log until both the CIL and AIL are empty. > */ > static int > -xfs_log_need_covered(xfs_mount_t *mp) > +xfs_log_need_covered( > + struct xfs_mount *mp) > { > - struct xlog *log = mp->m_log; > - int needed = 0; > - > - if (!xfs_fs_writable(mp, SB_FREEZE_WRITE)) > - return 0; > + struct xlog *log = mp->m_log; > + int needed = 0; > > if (!xlog_cil_empty(log)) > return 0; > @@ -1271,7 +1269,7 @@ xfs_log_worker( > struct xfs_mount *mp = log->l_mp; > > /* dgc: errors ignored - not fatal and nowhere to report them */ > - if (xfs_log_need_covered(mp)) { > + if (xfs_fs_writable(mp, SB_FREEZE_WRITE) && xfs_log_need_covered(mp)) { > /* > * Dump a transaction into the log that contains no real change. > * This is needed to stamp the current tail LSN into the log >
On Wed, Jan 06, 2021 at 12:41:20PM -0500, Brian Foster wrote: > The log covering helper checks whether the filesystem is writable to > determine whether to cover the log. The helper is currently only > called from the background log worker. In preparation to reuse the > helper from freezing contexts, lift the check into xfs_log_worker(). > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/xfs_log.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index b445e63cbc3c..4137ed007111 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1050,13 +1050,11 @@ xfs_log_space_wake( > * can't start trying to idle the log until both the CIL and AIL are empty. > */ > static int I think this is a predicate, right? Should this function return a bool instead of an int? This function always confuses me slightly since it pushes us through the covering state machine, and (I think) assumes that someone will force the CIL and push the AIL if it returns zero. :) To check my thinking further-- back in that thread I started about setting and clearing log incompat flags, I think Dave was pushing me to clear the log incompat flags just before we call xfs_sync_sb when the log is in NEED2 state, right? AFAICT the net effect of this series is to reorder the log code so that xfs_log_quiesce covers the log (force cil, push ail, log two transactions containing only the superblock), and adds an xfs_log_clean that quiesces the log and then writes an unmount record after that. Two callers whose behavior does not change with this series are: 1) The log worker quiesces the log when it's idle; and 2) unmount quiesces the log and then writes an unmount record so that the next mount knows it can skip replay entirely. The big difference is 3) freeze now only covers the log, whereas before it would cover, write an unmount record, and immediately redirty the log to force replay of the snapshot, right? Assuming I understood all that, my next question is: Eric Sandeen was working on a patchset to process unlinked inodes unconditionally on mount so that frozen fs images can be written out with the unmount record (because I guess people make ro snapshots of live fs images and then balk when they have to make the snapshot rw to run log recovery. Any thoughts about /that/? :) --D > -xfs_log_need_covered(xfs_mount_t *mp) > +xfs_log_need_covered( > + struct xfs_mount *mp) > { > - struct xlog *log = mp->m_log; > - int needed = 0; > - > - if (!xfs_fs_writable(mp, SB_FREEZE_WRITE)) > - return 0; > + struct xlog *log = mp->m_log; > + int needed = 0; > > if (!xlog_cil_empty(log)) > return 0; > @@ -1271,7 +1269,7 @@ xfs_log_worker( > struct xfs_mount *mp = log->l_mp; > > /* dgc: errors ignored - not fatal and nowhere to report them */ > - if (xfs_log_need_covered(mp)) { > + if (xfs_fs_writable(mp, SB_FREEZE_WRITE) && xfs_log_need_covered(mp)) { > /* > * Dump a transaction into the log that contains no real change. > * This is needed to stamp the current tail LSN into the log > -- > 2.26.2 >
On Thu, Jan 07, 2021 at 10:34:22AM -0800, Darrick J. Wong wrote: > On Wed, Jan 06, 2021 at 12:41:20PM -0500, Brian Foster wrote: > > The log covering helper checks whether the filesystem is writable to > > determine whether to cover the log. The helper is currently only > > called from the background log worker. In preparation to reuse the > > helper from freezing contexts, lift the check into xfs_log_worker(). > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/xfs/xfs_log.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > > index b445e63cbc3c..4137ed007111 100644 > > --- a/fs/xfs/xfs_log.c > > +++ b/fs/xfs/xfs_log.c > > @@ -1050,13 +1050,11 @@ xfs_log_space_wake( > > * can't start trying to idle the log until both the CIL and AIL are empty. > > */ > > static int > > I think this is a predicate, right? Should this function return a bool > instead of an int? > Yes, we could change that to return a bool. > This function always confuses me slightly since it pushes us through the > covering state machine, and (I think) assumes that someone will force > the CIL and push the AIL if it returns zero. :) > It basically assumes that the caller will issue a covering commit (xfs_sync_sb()) if indicated, and so progresses ->l_covered_state along in anticipation of that (i.e. NEED -> DONE). The log subsystem side detects that covering commit and makes further state changes (such as DONE -> NEED2) for the next time around in the background worker. > To check my thinking further-- back in that thread I started about > setting and clearing log incompat flags, I think Dave was pushing me to > clear the log incompat flags just before we call xfs_sync_sb when the > log is in NEED2 state, right? > In general, I think so. I don't think it technically has to be NEED2 (as opposed to NEED || NEED2), but in general the idea is to make any such final superblock updates in-core just before the quiesce completes and allow the log covering sequence to commit it for us. This is similar to how this series handles the lazy superblock counters (with the caveat that that stuff just happened to already be implemented inside xfs_sync_sb()). FWIW, we could also enforce that such final superblock updates reset covered state of the log to NEED2 if we wanted to. I went back and forth on that a bit but decided to leave out unnecessary complexity for the first pass. > AFAICT the net effect of this series is to reorder the log code so that > xfs_log_quiesce covers the log (force cil, push ail, log two > transactions containing only the superblock), and adds an xfs_log_clean > that quiesces the log and then writes an unmount record after that. > Yep. > Two callers whose behavior does not change with this series are: 1) The > log worker quiesces the log when it's idle; and 2) unmount quiesces the > log and then writes an unmount record so that the next mount knows it > can skip replay entirely. > Right, though just to be clear, quiesce never covered the log before this series. It effectively drained the log by forcing the log and pushing the AIL until empty, but then just wrote the unmount record to mark it clean... > The big difference is 3) freeze now only covers the log, whereas before > it would cover, write an unmount record, and immediately redirty the log > to force replay of the snapshot, right? > Yes. As above, unmount now also does a log cover -> unmount record instead of just writing the unmount record. This is harmless because we end up in the clean state either way, but I've tried to point this out in the commit logs and whatnot so it's apparent to reviewers. We could technically make the log cover during quiesce optional with a new parameter or something, but it just didn't seem worth it once we start overloading the covering sequence to handle things like lazy sb accounting (or log incompat bits, etc.). > Assuming I understood all that, my next question is: Eric Sandeen was > working on a patchset to process unlinked inodes unconditionally on > mount so that frozen fs images can be written out with the unmount > record (because I guess people make ro snapshots of live fs images and > then balk when they have to make the snapshot rw to run log recovery. > Any thoughts about /that/? :) > Eric had mentioned that to me as well. I don't quite recall what the impediment to making that change was the last time around (Eric?), but my view was that is orthogonal to this series. IOW, the primary motivations for this series are to clean up the whole xfs_quiesce_attr() -> xfs_log_quiesce() mess and facilitate the reuse of covering for things like lazy sb accounting and log incompat bit management. We can decide whether to quiesce or clean the log on freeze independently and that's really only a single line tweak to the last patch of the series (i.e., continue to clean the log and just don't redirty it). Brian > --D > > > -xfs_log_need_covered(xfs_mount_t *mp) > > +xfs_log_need_covered( > > + struct xfs_mount *mp) > > { > > - struct xlog *log = mp->m_log; > > - int needed = 0; > > - > > - if (!xfs_fs_writable(mp, SB_FREEZE_WRITE)) > > - return 0; > > + struct xlog *log = mp->m_log; > > + int needed = 0; > > > > if (!xlog_cil_empty(log)) > > return 0; > > @@ -1271,7 +1269,7 @@ xfs_log_worker( > > struct xfs_mount *mp = log->l_mp; > > > > /* dgc: errors ignored - not fatal and nowhere to report them */ > > - if (xfs_log_need_covered(mp)) { > > + if (xfs_fs_writable(mp, SB_FREEZE_WRITE) && xfs_log_need_covered(mp)) { > > /* > > * Dump a transaction into the log that contains no real change. > > * This is needed to stamp the current tail LSN into the log > > -- > > 2.26.2 > > >
On Thu, Jan 07, 2021 at 02:53:21PM -0500, Brian Foster wrote: > On Thu, Jan 07, 2021 at 10:34:22AM -0800, Darrick J. Wong wrote: > > On Wed, Jan 06, 2021 at 12:41:20PM -0500, Brian Foster wrote: > > > The log covering helper checks whether the filesystem is writable to > > > determine whether to cover the log. The helper is currently only > > > called from the background log worker. In preparation to reuse the > > > helper from freezing contexts, lift the check into xfs_log_worker(). > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > fs/xfs/xfs_log.c | 12 +++++------- > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > > > index b445e63cbc3c..4137ed007111 100644 > > > --- a/fs/xfs/xfs_log.c > > > +++ b/fs/xfs/xfs_log.c > > > @@ -1050,13 +1050,11 @@ xfs_log_space_wake( > > > * can't start trying to idle the log until both the CIL and AIL are empty. > > > */ > > > static int > > > > I think this is a predicate, right? Should this function return a bool > > instead of an int? > > > > Yes, we could change that to return a bool. > > > This function always confuses me slightly since it pushes us through the > > covering state machine, and (I think) assumes that someone will force > > the CIL and push the AIL if it returns zero. :) > > > > It basically assumes that the caller will issue a covering commit > (xfs_sync_sb()) if indicated, and so progresses ->l_covered_state along > in anticipation of that (i.e. NEED -> DONE). The log subsystem side > detects that covering commit and makes further state changes (such as > DONE -> NEED2) for the next time around in the background worker. > > > To check my thinking further-- back in that thread I started about > > setting and clearing log incompat flags, I think Dave was pushing me to > > clear the log incompat flags just before we call xfs_sync_sb when the > > log is in NEED2 state, right? > > > > In general, I think so. I don't think it technically has to be NEED2 (as > opposed to NEED || NEED2), but in general the idea is to make any such > final superblock updates in-core just before the quiesce completes and > allow the log covering sequence to commit it for us. This is similar to > how this series handles the lazy superblock counters (with the caveat > that that stuff just happened to already be implemented inside > xfs_sync_sb()). > > FWIW, we could also enforce that such final superblock updates reset > covered state of the log to NEED2 if we wanted to. I went back and forth > on that a bit but decided to leave out unnecessary complexity for the > first pass. > > > AFAICT the net effect of this series is to reorder the log code so that > > xfs_log_quiesce covers the log (force cil, push ail, log two > > transactions containing only the superblock), and adds an xfs_log_clean > > that quiesces the log and then writes an unmount record after that. > > > > Yep. > > > Two callers whose behavior does not change with this series are: 1) The > > log worker quiesces the log when it's idle; and 2) unmount quiesces the > > log and then writes an unmount record so that the next mount knows it > > can skip replay entirely. > > > > Right, though just to be clear, quiesce never covered the log before > this series. It effectively drained the log by forcing the log and > pushing the AIL until empty, but then just wrote the unmount record to > mark it clean... <nod> Right, I should've echoed that old quiesce only did force cil and push ail, so freeze and unmount do more now. > > The big difference is 3) freeze now only covers the log, whereas before > > it would cover, write an unmount record, and immediately redirty the log > > to force replay of the snapshot, right? > > > > Yes. As above, unmount now also does a log cover -> unmount record > instead of just writing the unmount record. This is harmless because we > end up in the clean state either way, but I've tried to point this out > in the commit logs and whatnot so it's apparent to reviewers. We could > technically make the log cover during quiesce optional with a new > parameter or something, but it just didn't seem worth it once we start > overloading the covering sequence to handle things like lazy sb > accounting (or log incompat bits, etc.). > > > Assuming I understood all that, my next question is: Eric Sandeen was > > working on a patchset to process unlinked inodes unconditionally on > > mount so that frozen fs images can be written out with the unmount > > record (because I guess people make ro snapshots of live fs images and > > then balk when they have to make the snapshot rw to run log recovery. > > Any thoughts about /that/? :) > > > > Eric had mentioned that to me as well. I don't quite recall what the > impediment to making that change was the last time around (Eric?), but > my view was that is orthogonal to this series. IOW, the primary > motivations for this series are to clean up the whole xfs_quiesce_attr() > -> xfs_log_quiesce() mess and facilitate the reuse of covering for > things like lazy sb accounting and log incompat bit management. We can > decide whether to quiesce or clean the log on freeze independently and > that's really only a single line tweak to the last patch of the series > (i.e., continue to clean the log and just don't redirty it). Oh, it's totally orthogonal, but touched some of the same code parts. :) IIRC I applied it then hit fstests regressions and kicked it out again. --D > Brian > > > --D > > > > > -xfs_log_need_covered(xfs_mount_t *mp) > > > +xfs_log_need_covered( > > > + struct xfs_mount *mp) > > > { > > > - struct xlog *log = mp->m_log; > > > - int needed = 0; > > > - > > > - if (!xfs_fs_writable(mp, SB_FREEZE_WRITE)) > > > - return 0; > > > + struct xlog *log = mp->m_log; > > > + int needed = 0; > > > > > > if (!xlog_cil_empty(log)) > > > return 0; > > > @@ -1271,7 +1269,7 @@ xfs_log_worker( > > > struct xfs_mount *mp = log->l_mp; > > > > > > /* dgc: errors ignored - not fatal and nowhere to report them */ > > > - if (xfs_log_need_covered(mp)) { > > > + if (xfs_fs_writable(mp, SB_FREEZE_WRITE) && xfs_log_need_covered(mp)) { > > > /* > > > * Dump a transaction into the log that contains no real change. > > > * This is needed to stamp the current tail LSN into the log > > > -- > > > 2.26.2 > > > > > >
On Wed, Jan 06, 2021 at 12:41:20PM -0500, Brian Foster wrote: > The log covering helper checks whether the filesystem is writable to > determine whether to cover the log. The helper is currently only > called from the background log worker. In preparation to reuse the > helper from freezing contexts, lift the check into xfs_log_worker(). > > Signed-off-by: Brian Foster <bfoster@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index b445e63cbc3c..4137ed007111 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1050,13 +1050,11 @@ xfs_log_space_wake( * can't start trying to idle the log until both the CIL and AIL are empty. */ static int -xfs_log_need_covered(xfs_mount_t *mp) +xfs_log_need_covered( + struct xfs_mount *mp) { - struct xlog *log = mp->m_log; - int needed = 0; - - if (!xfs_fs_writable(mp, SB_FREEZE_WRITE)) - return 0; + struct xlog *log = mp->m_log; + int needed = 0; if (!xlog_cil_empty(log)) return 0; @@ -1271,7 +1269,7 @@ xfs_log_worker( struct xfs_mount *mp = log->l_mp; /* dgc: errors ignored - not fatal and nowhere to report them */ - if (xfs_log_need_covered(mp)) { + if (xfs_fs_writable(mp, SB_FREEZE_WRITE) && xfs_log_need_covered(mp)) { /* * Dump a transaction into the log that contains no real change. * This is needed to stamp the current tail LSN into the log
The log covering helper checks whether the filesystem is writable to determine whether to cover the log. The helper is currently only called from the background log worker. In preparation to reuse the helper from freezing contexts, lift the check into xfs_log_worker(). Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_log.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)