Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
From: Brian Foster <bfoster@redhat.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Christoph Hellwig <hch@lst.de>,
	linux-xfs@vger.kernel.org, Dave Chinner <dchinner@redhat.com>
Subject: Re: [PATCH 1/8] xfs: don't try to write a start record into every iclog
Date: Thu, 26 Mar 2020 07:08:28 -0400
Message-ID: <20200326110828.GA19262@bfoster> (raw)
In-Reply-To: <20200325232500.GK10776@dread.disaster.area>

On Thu, Mar 26, 2020 at 10:25:00AM +1100, Dave Chinner wrote:
> On Wed, Mar 25, 2020 at 08:33:14AM -0400, Brian Foster wrote:
> > On Tue, Mar 24, 2020 at 06:44:52PM +0100, Christoph Hellwig wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The xlog_write() function iterates over iclogs until it completes
> > > writing all the log vectors passed in. The ticket tracks whether
> > > a start record has been written or not, so only the first iclog gets
> > > a start record. We only ever pass single use tickets to
> > > xlog_write() so we only ever need to write a start record once per
> > > xlog_write() call.
> > > 
> > > Hence we don't need to store whether we should write a start record
> > > in the ticket as the callers provide all the information we need to
> > > determine if a start record should be written. For the moment, we
> > > have to ensure that we clear the XLOG_TIC_INITED appropriately so
> > > the code in xfs_log_done() still works correctly for committing
> > > transactions.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > [hch: use an need_start_rec bool]
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_log.c | 63 ++++++++++++++++++++++++------------------------
> > >  1 file changed, 32 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > index 2a90a483c2d6..bf071552094a 100644
> > > --- a/fs/xfs/xfs_log.c
> > > +++ b/fs/xfs/xfs_log.c
> > ...
> > > @@ -2372,25 +2359,29 @@ xlog_write(
> > >  	int			record_cnt = 0;
> > >  	int			data_cnt = 0;
> > >  	int			error = 0;
> > > +	bool			need_start_rec = true;
> > >  
> > >  	*start_lsn = 0;
> > >  
> > > -	len = xlog_write_calc_vec_length(ticket, log_vector);
> > >  
> > >  	/*
> > >  	 * Region headers and bytes are already accounted for.
> > >  	 * We only need to take into account start records and
> > >  	 * split regions in this function.
> > >  	 */
> > > -	if (ticket->t_flags & XLOG_TIC_INITED)
> > > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > > +	if (ticket->t_flags & XLOG_TIC_INITED) {
> > > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > > +		ticket->t_flags &= ~XLOG_TIC_INITED;
> > > +	}
> > >  
> > >  	/*
> > >  	 * Commit record headers need to be accounted for. These
> > >  	 * come in as separate writes so are easy to detect.
> > >  	 */
> > > -	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS))
> > > -		ticket->t_curr_res -= sizeof(xlog_op_header_t);
> > > +	if (flags & (XLOG_COMMIT_TRANS | XLOG_UNMOUNT_TRANS)) {
> > > +		ticket->t_curr_res -= sizeof(struct xlog_op_header);
> > > +		need_start_rec = false;
> > > +	}
> > 
> > Hmm.. I was asking for a comment update in v1 for this logic change.
> > Looking through it again, what happens here if
> > xfs_log_write_unmount_record() clears the UNMOUNT_TRANS flag for that
> > summary counter check? That looks like a potential behavior change wrt
> > to the start record..
> 
> xfs_log_write_unmount_record() clears the XLOG_TIC_INITED
> flag before calling xlog_write(), so the current code never writes
> out a start record for the unmount record. i.e. the unmount
> record is a single region with the unmount log item in it, and
> AFAICT this code does not change the behaviour of the unmount record
> write at all.
> 

I'm referring to the UNMOUNT_TRANS flag, not t_flags. With this patch,
we actually would write a start record in some cases because
need_start_rec is toggled based on the flags parameter and the summary
counter check zeroes it.

> FWIW, that error injection code looks dodgy - it turns the unmount
> record into an XFS_LOG transaction type with an invalid log item
> type (0). That probably should be flagged as corruption, not be
> silently ignored by recovery. IOWs, I think the error injection code
> was wrong to begin with - if we want to ensure the log is dirty at
> unmount, we should just skip writing the unmount record altogether.
> 

That may be true... TBH I wasn't totally clear on what that logic was
for (it isn't purely error injection). From the commit (f467cad95f5e3)
log, the intent appears to be to "skip writing the unmount record," but
that doesn't quite describe the behavior. Darrick might want to
comment..? If we do revisit this, I'm mainly curious on whether there's
a change in recovery behavior between having this specially crafted
record vs. just writing nothing. For example, does recovery still set
the head/tail based on this record even though we don't mark the log
clean? If so, do we care..?

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 


  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 17:44 xfs: clean up log tickets and record writes v3 Christoph Hellwig
2020-03-24 17:44 ` [PATCH 1/8] xfs: don't try to write a start record into every iclog Christoph Hellwig
2020-03-24 20:39   ` Darrick J. Wong
2020-03-25 12:33   ` Brian Foster
2020-03-25 23:25     ` Dave Chinner
2020-03-26 11:08       ` Brian Foster [this message]
2020-03-26 15:41         ` Darrick J. Wong
2020-03-24 17:44 ` [PATCH 2/8] xfs: re-order initial space accounting checks in xlog_write Christoph Hellwig
2020-03-24 20:39   ` Darrick J. Wong
2020-03-24 17:44 ` [PATCH 3/8] xfs: refactor and split xfs_log_done() Christoph Hellwig
2020-03-24 20:39   ` Darrick J. Wong
2020-03-25 12:33   ` Brian Foster
2020-03-24 17:44 ` [PATCH 4/8] xfs: kill XLOG_TIC_INITED Christoph Hellwig
2020-03-24 20:40   ` Darrick J. Wong
2020-03-24 17:44 ` [PATCH 5/8] xfs: split xlog_ticket_done Christoph Hellwig
2020-03-24 20:41   ` Darrick J. Wong
2020-03-25 12:34   ` Brian Foster
2020-03-24 17:44 ` [PATCH 6/8] xfs: merge xlog_commit_record with xlog_write_done() Christoph Hellwig
2020-03-24 20:41   ` Darrick J. Wong
2020-03-24 17:44 ` [PATCH 7/8] xfs: refactor unmount record writing Christoph Hellwig
2020-03-24 20:41   ` Darrick J. Wong
2020-03-24 17:44 ` [PATCH 8/8] xfs: remove some stale comments from the log code Christoph Hellwig
2020-03-24 20:42   ` Darrick J. Wong
2020-03-25 18:42 xfs: clean up log tickets and record writes v4 Christoph Hellwig
2020-03-25 18:42 ` [PATCH 1/8] xfs: don't try to write a start record into every iclog Christoph Hellwig
2020-03-26  4:49   ` Darrick J. Wong
2020-03-26 11:09   ` Brian Foster

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=20200326110828.GA19262@bfoster \
    --to=bfoster@redhat.com \
    --cc=david@fromorbit.com \
    --cc=dchinner@redhat.com \
    --cc=hch@lst.de \
    --cc=linux-xfs@vger.kernel.org \
    /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

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-xfs linux-xfs/ https://lore.kernel.org/linux-xfs \
		linux-xfs@vger.kernel.org
	public-inbox-index linux-xfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git