<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi,<br>
</p>
<div class="moz-cite-prefix">On 20/08/2020 12:04, Abhijith Das
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CACrDRjiVxrtX_tLmO+Ym=gbCrsndvOYp42z34ESbWNsiB5XP6w@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_default"
style="font-family:monospace,monospace"><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Aug 19, 2020 at
12:07 PM Bob Peterson <<a
href="mailto:rpeterso@redhat.com" moz-do-not-send="true">rpeterso@redhat.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">----- Original Message
-----<br>
> We store the local statfs info in the journal header
now so<br>
> there's no need to write to the local statfs file
anymore.<br>
> <br>
> Signed-off-by: Abhi Das <<a
href="mailto:adas@redhat.com" target="_blank"
moz-do-not-send="true">adas@redhat.com</a>><br>
> ---<br>
> fs/gfs2/lops.c | 10 +++++++++-<br>
> 1 file changed, 9 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c<br>
> index cb2a11b458c6..53d2dbf6605e 100644<br>
> --- a/fs/gfs2/lops.c<br>
> +++ b/fs/gfs2/lops.c<br>
> @@ -104,7 +104,15 @@ static void gfs2_unpin(struct
gfs2_sbd *sdp, struct<br>
> buffer_head *bh,<br>
> BUG_ON(!buffer_pinned(bh));<br>
> <br>
> lock_buffer(bh);<br>
> - mark_buffer_dirty(bh);<br>
> + /*<br>
> + * We want to eliminate the local statfs file
eventually.<br>
> + * But, for now, we're simply not going to update
it by<br>
> + * never marking its buffers dirty<br>
> + */<br>
> + if (!(bd->bd_gl->gl_name.ln_type ==
LM_TYPE_INODE &&<br>
> + bd->bd_gl->gl_object ==
GFS2_I(sdp->sd_sc_inode)))<br>
> + mark_buffer_dirty(bh);<br>
> +<br>
> clear_buffer_pinned(bh);<br>
> <br>
> if (buffer_is_rgrp(bd))<br>
> --<br>
> 2.20.1<br>
<br>
Hi,<br>
<br>
This seems dangerous to me. It can only get to gfs2_unpin by
trying to<br>
commit buffers for a transaction. If the buffers aren't
marked dirty,<br>
that means transactions will be queued to the ail1 list that
won't be<br>
fully written. So what happens to them? Do they eventually
get freed?<br>
<br>
I'm also concerned about a potential impact to performance,
since<br>
gfs2_unpin gets called with every metadata buffer that's
used.<br>
The additional if checks may not costs us much time-wise,
but it's a<br>
pretty hot function.<br>
<br>
Can't we accomplish the same thing by making function
update_statfs()<br>
never add the buffers to the transaction in the first place?<br>
IOW, by just removing the line:<br>
gfs2_trans_add_meta(m_ip->i_gl, m_bh);<br>
That way we don't need to worry about its buffer getting
pinned,<br>
unpinned and queued to the ail.<br>
<br>
Regards,<br>
<br>
Bob Peterson<br>
<br>
</blockquote>
<div> </div>
<div><span class="gmail_default"
style="font-family:monospace,monospace">Fair point. I'll
post an updated version of this patch that </span> <span
class="gmail_default"
style="font-family:monospace,monospace">doesn't queue the
buffer in the first place.</span></div>
<div><span class="gmail_default"
style="font-family:monospace,monospace"><br>
</span></div>
<div><span class="gmail_default"
style="font-family:monospace,monospace">Cheers!<br>
--Abhi</span></div>
</div>
</div>
</blockquote>
<p>You need to think about correctness at recovery time. It may be
faster to not write the data into the journal for the local statfs
file, but how will that affect recovery depending on whether that
recovery is performed by either a newer or older kernel? Being
backwards compatible might be more important in this case, so
worth looking at carefully,</p>
<p>Steve.</p>
<p><br>
</p>
</body>
</html>