Re: [Cluster-devel] [PATCH 1/3] gfs2: Don't write updates to local statfs file

[Steven Whitehouse]

Hi,

On 20/08/2020 12:04, Abhijith Das wrote:



On Wed, Aug 19, 2020 at 12:07 PM Bob Peterson > wrote:


- Original Message -
> We store the local statfs info in the journal header now so
> there's no need to write to the local statfs file anymore.
>
> Signed-off-by: Abhi Das mailto:a...@redhat.com>>
> ---
>  fs/gfs2/lops.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index cb2a11b458c6..53d2dbf6605e 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -104,7 +104,15 @@ static void gfs2_unpin(struct gfs2_sbd
*sdp, struct
> buffer_head *bh,
>       BUG_ON(!buffer_pinned(bh));
>
>       lock_buffer(bh);
> -     mark_buffer_dirty(bh);
> +     /*
> +      * We want to eliminate the local statfs file eventually.
> +      * But, for now, we're simply not going to update it by
> +      * never marking its buffers dirty
> +      */
> +     if (!(bd->bd_gl->gl_name.ln_type == LM_TYPE_INODE &&
> +           bd->bd_gl->gl_object == GFS2_I(sdp->sd_sc_inode)))
> +             mark_buffer_dirty(bh);
> +
>       clear_buffer_pinned(bh);
>
>       if (buffer_is_rgrp(bd))
> --
> 2.20.1

Hi,

This seems dangerous to me. It can only get to gfs2_unpin by trying to
commit buffers for a transaction. If the buffers aren't marked dirty,
that means transactions will be queued to the ail1 list that won't be
fully written. So what happens to them? Do they eventually get freed?

I'm also concerned about a potential impact to performance, since
gfs2_unpin gets called with every metadata buffer that's used.
The additional if checks may not costs us much time-wise, but it's a
pretty hot function.

Can't we accomplish the same thing by making function update_statfs()
never add the buffers to the transaction in the first place?
IOW, by just removing the line:
        gfs2_trans_add_meta(m_ip->i_gl, m_bh);
That way we don't need to worry about its buffer getting pinned,
unpinned and queued to the ail.

Regards,

Bob Peterson

Fair point. I'll post an updated version of this patch that doesn't 
queue the buffer in the first place.


Cheers!
--Abhi


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,


Steve.

Re: [Cluster-devel] [PATCH 1/3] gfs2: Don't write updates to local statfs file

[Abhijith Das]

On Wed, Aug 19, 2020 at 12:07 PM Bob Peterson  wrote:

> - Original Message -
> > We store the local statfs info in the journal header now so
> > there's no need to write to the local statfs file anymore.
> >
> > Signed-off-by: Abhi Das 
> > ---
> >  fs/gfs2/lops.c | 10 +-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> > index cb2a11b458c6..53d2dbf6605e 100644
> > --- a/fs/gfs2/lops.c
> > +++ b/fs/gfs2/lops.c
> > @@ -104,7 +104,15 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct
> > buffer_head *bh,
> >   BUG_ON(!buffer_pinned(bh));
> >
> >   lock_buffer(bh);
> > - mark_buffer_dirty(bh);
> > + /*
> > +  * We want to eliminate the local statfs file eventually.
> > +  * But, for now, we're simply not going to update it by
> > +  * never marking its buffers dirty
> > +  */
> > + if (!(bd->bd_gl->gl_name.ln_type == LM_TYPE_INODE &&
> > +   bd->bd_gl->gl_object == GFS2_I(sdp->sd_sc_inode)))
> > + mark_buffer_dirty(bh);
> > +
> >   clear_buffer_pinned(bh);
> >
> >   if (buffer_is_rgrp(bd))
> > --
> > 2.20.1
>
> Hi,
>
> This seems dangerous to me. It can only get to gfs2_unpin by trying to
> commit buffers for a transaction. If the buffers aren't marked dirty,
> that means transactions will be queued to the ail1 list that won't be
> fully written. So what happens to them? Do they eventually get freed?
>
> I'm also concerned about a potential impact to performance, since
> gfs2_unpin gets called with every metadata buffer that's used.
> The additional if checks may not costs us much time-wise, but it's a
> pretty hot function.
>
> Can't we accomplish the same thing by making function update_statfs()
> never add the buffers to the transaction in the first place?
> IOW, by just removing the line:
> gfs2_trans_add_meta(m_ip->i_gl, m_bh);
> That way we don't need to worry about its buffer getting pinned,
> unpinned and queued to the ail.
>
> Regards,
>
> Bob Peterson
>
>
Fair point. I'll post an updated version of this patch that  doesn't queue
the buffer in the first place.

Cheers!
--Abhi

Re: [Cluster-devel] [PATCH 1/3] gfs2: Don't write updates to local statfs file

[Bob Peterson]

- Original Message -
> We store the local statfs info in the journal header now so
> there's no need to write to the local statfs file anymore.
> 
> Signed-off-by: Abhi Das 
> ---
>  fs/gfs2/lops.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index cb2a11b458c6..53d2dbf6605e 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -104,7 +104,15 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct
> buffer_head *bh,
>   BUG_ON(!buffer_pinned(bh));
>  
>   lock_buffer(bh);
> - mark_buffer_dirty(bh);
> + /*
> +  * We want to eliminate the local statfs file eventually.
> +  * But, for now, we're simply not going to update it by
> +  * never marking its buffers dirty
> +  */
> + if (!(bd->bd_gl->gl_name.ln_type == LM_TYPE_INODE &&
> +   bd->bd_gl->gl_object == GFS2_I(sdp->sd_sc_inode)))
> + mark_buffer_dirty(bh);
> +
>   clear_buffer_pinned(bh);
>  
>   if (buffer_is_rgrp(bd))
> --
> 2.20.1

Hi,

This seems dangerous to me. It can only get to gfs2_unpin by trying to
commit buffers for a transaction. If the buffers aren't marked dirty,
that means transactions will be queued to the ail1 list that won't be
fully written. So what happens to them? Do they eventually get freed?

I'm also concerned about a potential impact to performance, since
gfs2_unpin gets called with every metadata buffer that's used.
The additional if checks may not costs us much time-wise, but it's a
pretty hot function.

Can't we accomplish the same thing by making function update_statfs()
never add the buffers to the transaction in the first place?
IOW, by just removing the line:
gfs2_trans_add_meta(m_ip->i_gl, m_bh);
That way we don't need to worry about its buffer getting pinned,
unpinned and queued to the ail.

Regards,

Bob Peterson

[Cluster-devel] [PATCH 1/3] gfs2: Don't write updates to local statfs file

[Abhi Das]

We store the local statfs info in the journal header now so
there's no need to write to the local statfs file anymore.

Signed-off-by: Abhi Das 
---
 fs/gfs2/lops.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index cb2a11b458c6..53d2dbf6605e 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -104,7 +104,15 @@ static void gfs2_unpin(struct gfs2_sbd *sdp, struct 
buffer_head *bh,
BUG_ON(!buffer_pinned(bh));
 
lock_buffer(bh);
-   mark_buffer_dirty(bh);
+   /*
+* We want to eliminate the local statfs file eventually.
+* But, for now, we're simply not going to update it by
+* never marking its buffers dirty
+*/
+   if (!(bd->bd_gl->gl_name.ln_type == LM_TYPE_INODE &&
+ bd->bd_gl->gl_object == GFS2_I(sdp->sd_sc_inode)))
+   mark_buffer_dirty(bh);
+
clear_buffer_pinned(bh);
 
if (buffer_is_rgrp(bd))
-- 
2.20.1