linux-next: build warning after merge of the xfs tree

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:

fs/xfs/xfs_buf.c: In function 'xfs_buf_bio_end_io':
fs/xfs/xfs_buf.c:1292:20: warning: unused variable 'mp' [-Wunused-variable]
 1292 |  struct xfs_mount *mp = bp->b_mount;
      |                    ^~

Introduced by commit

  7376d7454734 ("xfs: random buffer write failure errortag")

DEBUG is not defined.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warning after merge of the xfs tree

[Christoph Hellwig]

On Sat, Feb 24, 2024 at 06:07:28PM +0900, Akira Yokosawa wrote:
> How about teaching kernel-doc unary "*" on param?

That would be great!

> 
> Substitution would look like:
> 
>    (kernel-doc)       (RST)
>    *@param     ->  ***param**
> 
> Sphinx detects double asterisk, starts strong emphasis, waits for
> another double asterisk to appear, and stops strong emphasis.
> Hence you would get boldface "*param" in pretty printed docs.
> 
> Diff below (against docs-next) should add a rule for param_deref
> (only for RST).

I'd love to go with this instead of working around the issue:

Acked-by: Christoph Hellwig <hch@lst.de>

Re: linux-next: build warning after merge of the xfs tree

[Akira Yokosawa]

Hi,

On Fri, 23 Feb 2024 15:06:19 +0100, Christoph Hellwig wrote:
> On Fri, Feb 23, 2024 at 09:55:09AM +0100, Mauro Carvalho Chehab wrote:
>> but it is very weird for the ones reading the text file. So, what
>> we do instead for pointers is to escape the entire declaration, like:
>> 
>> 	``*inode``
>> 	``struct inode *inode``
>> 
>> I hope that helps.
> 
> In this case it says *foliop for an argument that is a double pointer
> and the comment refers to what it point to.  I'll see what I can do
> there, but the whole italic and bold thing seems entirely pointless
> for kerneldoc..

Indeed.

How about teaching kernel-doc unary "*" on param?

Substitution would look like:

   (kernel-doc)       (RST)
   *@param     ->  ***param**

Sphinx detects double asterisk, starts strong emphasis, waits for
another double asterisk to appear, and stops strong emphasis.
Hence you would get boldface "*param" in pretty printed docs.

Diff below (against docs-next) should add a rule for param_deref
(only for RST).

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 136104804375..bdd6f3b489cc 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -65,7 +65,7 @@ my $type_constant = '\b``([^\`]+)``\b';
 my $type_constant2 = '\%([-_\*\w]+)';
 my $type_func = '(\w+)\(\)';
 my $type_param = '\@(\w*((\.\w+)|(->\w+))*(\.\.\.)?)';
-my $type_param_ref = '([\!~]?)\@(\w*((\.\w+)|(->\w+))*(\.\.\.)?)';
+my $type_param_ref = '([\!~\*]?)\@(\w*((\.\w+)|(->\w+))*(\.\.\.)?)';
 my $type_fp_param = '\@(\w+)\(\)';  # Special RST handling for func ptr params
 my $type_fp_param2 = '\@(\w+->\S+)\(\)';  # Special RST handling for structs with func ptr params
 my $type_env = '(\$\w+)';
-- 

And you would be able to write the kernel-doc comment in question
as follows:

diff --git a/mm/shmem.c b/mm/shmem.c
index 750ab1dcae27..0aad0d9a621b 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2152,8 +2152,8 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index,
  * There is no need to reserve space before calling folio_mark_dirty().
  *
  * When no folio is found, the behavior depends on @sgp:
- *  - for SGP_READ, *foliop is %NULL and 0 is returned
- *  - for SGP_NOALLOC, *foliop is %NULL and -ENOENT is returned
+ *  - for SGP_READ, *@foliop is %NULL and 0 is returned
+ *  - for SGP_NOALLOC, *@foliop is %NULL and -ENOENT is returned
  *  - for all other flags a new folio is allocated, inserted into the
  *    page cache and returned locked in @foliop.
  *
-- 

How does this approach sound to you? 

        Thanks, Akira

Re: linux-next: build warning after merge of the xfs tree

[Christoph Hellwig]

On Fri, Feb 23, 2024 at 09:55:09AM +0100, Mauro Carvalho Chehab wrote:
> but it is very weird for the ones reading the text file. So, what
> we do instead for pointers is to escape the entire declaration, like:
> 
> 	``*inode``
> 	``struct inode *inode``
> 
> I hope that helps.

In this case it says *foliop for an argument that is a double pointer
and the comment refers to what it point to.  I'll see what I can do
there, but the whole italic and bold thing seems entirely pointless
for kerneldoc..

Re: linux-next: build warning after merge of the xfs tree

[Mauro Carvalho Chehab]

Em Fri, 23 Feb 2024 07:35:54 +0100
Christoph Hellwig <hch@lst.de> escreveu:

> On Fri, Feb 23, 2024 at 03:36:36PM +1100, Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the xfs tree, today's linux-next build (htmldocs) produced
> > this warning:
> > 
> > Documentation/core-api/mm-api:130: mm/shmem.c:2155: WARNING: Inline emphasis start-string without end-string.
> > Documentation/core-api/mm-api:130: mm/shmem.c:2156: WARNING: Inline emphasis start-string without end-string.  
> 
> Thanks for the headsup.  Looks like kernel-doc doesn't like the
> pointer dereferene * operator.  Any good idea how to write this instead?

Asterisks there are used to do *italic* or **bold**. To have an
asterisk as-is, it needs to be escaped. This would work:

	\*foo

but it is very weird for the ones reading the text file. So, what
we do instead for pointers is to escape the entire declaration, like:

	``*inode``
	``struct inode *inode``

I hope that helps.

Thanks,
Mauro

Re: linux-next: build warning after merge of the xfs tree

[Christoph Hellwig]

On Fri, Feb 23, 2024 at 03:36:36PM +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the xfs tree, today's linux-next build (htmldocs) produced
> this warning:
> 
> Documentation/core-api/mm-api:130: mm/shmem.c:2155: WARNING: Inline emphasis start-string without end-string.
> Documentation/core-api/mm-api:130: mm/shmem.c:2156: WARNING: Inline emphasis start-string without end-string.

Thanks for the headsup.  Looks like kernel-doc doesn't like the
pointer dereferene * operator.  Any good idea how to write this instead?

> 
> Introduced by commit
> 
>   d7468609ee0f ("shmem: export shmem_get_folio")
> 
> -- 
> Cheers,
> Stephen Rothwell


---end quoted text---

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build (htmldocs) produced
this warning:

Documentation/core-api/mm-api:130: mm/shmem.c:2155: WARNING: Inline emphasis start-string without end-string.
Documentation/core-api/mm-api:130: mm/shmem.c:2156: WARNING: Inline emphasis start-string without end-string.

Introduced by commit

  d7468609ee0f ("shmem: export shmem_get_folio")

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warning after merge of the xfs tree

[Dave Chinner]

On Mon, Jun 07, 2021 at 10:48:19AM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the xfs tree, today's linux-next build (powerpc_ppc64
> defconfig) produced this warning:
> 
> fs/xfs/libxfs/xfs_ialloc.c: In function 'xfs_difree_finobt':
> fs/xfs/libxfs/xfs_ialloc.c:2032:20: warning: unused variable 'agi' [-Wunused-variable]
>  2032 |  struct xfs_agi   *agi = agbp->b_addr;
>       |                    ^~~
> 
> Not sure how this came about, but somehow DEBUG has been turned off
> which exposes this.

I think I replaced all the agi->agi_seqno usages in that function
with pag->pag_agno and so now the agi structure is only accessed via
debug functions. The debug code should now pass perag structures
rather than raw AGI structures to check the free inode counts....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build (powerpc_ppc64
defconfig) produced this warning:

fs/xfs/libxfs/xfs_ialloc.c: In function 'xfs_difree_finobt':
fs/xfs/libxfs/xfs_ialloc.c:2032:20: warning: unused variable 'agi' [-Wunused-variable]
 2032 |  struct xfs_agi   *agi = agbp->b_addr;
      |                    ^~~

Not sure how this came about, but somehow DEBUG has been turned off
which exposes this.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build (htmldocs) produced
this warning:

Documentation/admin-guide/xfs.rst:531: WARNING: Malformed table.
No bottom table border found or no blank line after table bottom.

============     ===========
  Knob           Description
============     ===========
Documentation/admin-guide/xfs.rst:534: WARNING: Blank line required after table.
Documentation/admin-guide/xfs.rst:536: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/admin-guide/xfs.rst:538: WARNING: Unexpected indentation.

Introduced by commit

  f83d436aef5d ("xfs: increase the default parallelism levels of pwork clients")

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warning after merge of the xfs tree

[Darrick J. Wong]

On Mon, Jan 25, 2021 at 08:26:16AM -0500, Brian Foster wrote:
> On Mon, Jan 25, 2021 at 09:55:32AM +1100, Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the xfs tree, today's linux-next build (powerpc
> > ppc64_defconfig) produced this warning:
> > 
> > fs/xfs/xfs_log.c: In function 'xfs_log_cover':
> > fs/xfs/xfs_log.c:1111:16: warning: unused variable 'log' [-Wunused-variable]
> >  1111 |  struct xlog  *log = mp->m_log;
> >       |                ^~~
> > 
> > Introduced by commit
> > 
> >   303591a0a947 ("xfs: cover the log during log quiesce")
> > 
> 
> Oops, patch below. Feel free to apply or squash into the original
> commit.
> 
> Brian
> 
> --- 8< ---
> 
> From 6078f06e2bd4c82111a85a2032c39a56654b0be6 Mon Sep 17 00:00:00 2001
> From: Brian Foster <bfoster@redhat.com>
> Date: Mon, 25 Jan 2021 08:22:56 -0500
> Subject: [PATCH] xfs: fix unused log variable in xfs_log_cover()
> 
> The log variable is only used in kernels with asserts enabled.
> Remove it and open code the dereference to avoid unused variable
> warnings.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok to me...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 58699881c100..d8b814227734 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1108,12 +1108,11 @@ static int
>  xfs_log_cover(
>  	struct xfs_mount	*mp)
>  {
> -	struct xlog		*log = mp->m_log;
>  	int			error = 0;
>  	bool			need_covered;
>  
> -	ASSERT((xlog_cil_empty(log) && xlog_iclogs_empty(log) &&
> -	        !xfs_ail_min_lsn(log->l_ailp)) ||
> +	ASSERT((xlog_cil_empty(mp->m_log) && xlog_iclogs_empty(mp->m_log) &&
> +	        !xfs_ail_min_lsn(mp->m_log->l_ailp)) ||
>  	       XFS_FORCED_SHUTDOWN(mp));
>  
>  	if (!xfs_log_writable(mp))
> -- 
> 2.26.2
> 

Re: linux-next: build warning after merge of the xfs tree

[Brian Foster]

On Mon, Jan 25, 2021 at 09:55:32AM +1100, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the xfs tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
> 
> fs/xfs/xfs_log.c: In function 'xfs_log_cover':
> fs/xfs/xfs_log.c:1111:16: warning: unused variable 'log' [-Wunused-variable]
>  1111 |  struct xlog  *log = mp->m_log;
>       |                ^~~
> 
> Introduced by commit
> 
>   303591a0a947 ("xfs: cover the log during log quiesce")
> 

Oops, patch below. Feel free to apply or squash into the original
commit.

Brian

--- 8< ---

From 6078f06e2bd4c82111a85a2032c39a56654b0be6 Mon Sep 17 00:00:00 2001
From: Brian Foster <bfoster@redhat.com>
Date: Mon, 25 Jan 2021 08:22:56 -0500
Subject: [PATCH] xfs: fix unused log variable in xfs_log_cover()

The log variable is only used in kernels with asserts enabled.
Remove it and open code the dereference to avoid unused variable
warnings.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_log.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 58699881c100..d8b814227734 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1108,12 +1108,11 @@ static int
 xfs_log_cover(
 	struct xfs_mount	*mp)
 {
-	struct xlog		*log = mp->m_log;
 	int			error = 0;
 	bool			need_covered;
 
-	ASSERT((xlog_cil_empty(log) && xlog_iclogs_empty(log) &&
-	        !xfs_ail_min_lsn(log->l_ailp)) ||
+	ASSERT((xlog_cil_empty(mp->m_log) && xlog_iclogs_empty(mp->m_log) &&
+	        !xfs_ail_min_lsn(mp->m_log->l_ailp)) ||
 	       XFS_FORCED_SHUTDOWN(mp));
 
 	if (!xfs_log_writable(mp))
-- 
2.26.2

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:

fs/xfs/xfs_log.c: In function 'xfs_log_cover':
fs/xfs/xfs_log.c:1111:16: warning: unused variable 'log' [-Wunused-variable]
 1111 |  struct xlog  *log = mp->m_log;
      |                ^~~

Introduced by commit

  303591a0a947 ("xfs: cover the log during log quiesce")

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warning after merge of the xfs tree

[Darrick J. Wong]

On Fri, Jan 24, 2020 at 01:47:22PM +1100, Stephen Rothwell wrote:
> Hi all
> 
> On Thu, 16 Jan 2020 09:12:42 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > After merging the xfs tree, today's linux-next build
> > (powerpppc64_defconfig) produced this warning:
> > 
> > fs/xfs/xfs_inode.c: In function 'xfs_itruncate_extents_flags':
> > fs/xfs/xfs_inode.c:1523:8: warning: unused variable 'done' [-Wunused-variable]
> >  1523 |  int   done = 0;
> >       |        ^~~~
> > 
> > Introduced by commit
> > 
> >   4bbb04abb4ee ("xfs: truncate should remove all blocks, not just to the end of the page cache")
> 
> I am still getting this warning.

Aha, I had though this was gone but it turns out that I accidentally
merged the fix for this into an internal patch to grease my internal CI
scripts.  Ah well.  Patch soon.

--D

> 
> -- 
> Cheers,
> Stephen Rothwell

Re: linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

Hi all

On Thu, 16 Jan 2020 09:12:42 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the xfs tree, today's linux-next build
> (powerpppc64_defconfig) produced this warning:
> 
> fs/xfs/xfs_inode.c: In function 'xfs_itruncate_extents_flags':
> fs/xfs/xfs_inode.c:1523:8: warning: unused variable 'done' [-Wunused-variable]
>  1523 |  int   done = 0;
>       |        ^~~~
> 
> Introduced by commit
> 
>   4bbb04abb4ee ("xfs: truncate should remove all blocks, not just to the end of the page cache")

I am still getting this warning.


-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build
(powerpppc64_defconfig) produced this warning:

fs/xfs/xfs_inode.c: In function 'xfs_itruncate_extents_flags':
fs/xfs/xfs_inode.c:1523:8: warning: unused variable 'done' [-Wunused-variable]
 1523 |  int   done = 0;
      |        ^~~~

Introduced by commit

  4bbb04abb4ee ("xfs: truncate should remove all blocks, not just to the end of the page cache")



-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

Hi all,

After merging the xfs tree, today's linux-next build
(powerpppc64_defconfig) produced this warning:

fs/xfs/xfs_inode.c: In function 'xfs_itruncate_extents_flags':
fs/xfs/xfs_inode.c:1523:8: warning: unused variable 'done' [-Wunused-variable]
 1523 |  int   done = 0;
      |        ^~~~

Introduced by commit

  4bbb04abb4ee ("xfs: truncate should remove all blocks, not just to the end of the page cache")

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

Hi all,

After merging the xfs tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:

fs/xfs/libxfs/xfs_bmap.c: In function 'xfs_bmap_del_extent_delay':
fs/xfs/libxfs/xfs_bmap.c:4648:20: warning: unused variable 'ifp' [-Wunused-variable]
  struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
                    ^
fs/xfs/libxfs/xfs_bmap.c: In function 'xfs_bmap_del_extent_cow':
fs/xfs/libxfs/xfs_bmap.c:4776:20: warning: unused variable 'ifp' [-Wunused-variable]
  struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
                    ^

Introduced by commit

  ca5d8e5b7b90 ("xfs: move pre/post-bmap tracing into xfs_iext_update_extent")

-- 
Cheers,
Stephen Rothwell

Re: linux-next: build warning after merge of the xfs tree

[Dave Chinner]

On Thu, Aug 31, 2017 at 11:22:20AM -0400, Brian Foster wrote:
> On Thu, Aug 31, 2017 at 07:57:52AM -0700, Darrick J. Wong wrote:
> > On Thu, Aug 31, 2017 at 06:30:41AM -0400, Brian Foster wrote:
> > > On Thu, Aug 31, 2017 at 10:07:03AM +1000, Stephen Rothwell wrote:
> > > > Hi all,
> > > > 
> > > > After merging the xfs tree, today's linux-next build (powerpc
> > > > ppc64_defconfig) produced this warning:
> > > > 
> > > > fs/xfs/xfs_buf_item.c: In function 'xfs_buf_item_unlock':
> > > > fs/xfs/xfs_buf_item.c:573:9: warning: unused variable 'ordered' [-Wunused-variable]
> > > >   bool   ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > > >          ^
> > > > 
> > > > Introduced by commit
> > > > 
> > > >   a097077ef708 ("xfs: remove unnecessary dirty bli format check for ordered bufs")
> > > > 
> > > 
> > > Ugh, this is due to the refactoring of this patch between v1 and v2. I
> > > specifically recall testing for this in v1 because I added the ordered
> > > bool purely to clean up the ASSERT(), then I apparently lost of track of
> > > it for v2.
> > > 
> > > Anyways.. Christoph, Darrick, preferences to clean this up..? I have no
> > > preference between the v1 or v2 factoring. Or if it's easier, we could
> > > always just drop something like the hunk below on top. Thoughts?
> > > 
> > > Brian
> > > 
> > > --- 8< ---
> > > 
> > > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > > index ef2c137..f5d25f5 100644
> > > --- a/fs/xfs/xfs_buf_item.c
> > > +++ b/fs/xfs/xfs_buf_item.c
> > > @@ -567,10 +567,15 @@ xfs_buf_item_unlock(
> > >  {
> > >  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
> > >  	struct xfs_buf		*bp = bip->bli_buf;
> > > -	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> > > -	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> > > -	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> > > -	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > > +	bool			aborted;
> > > +	bool			hold;
> > > +	bool			dirty;
> > > +	bool			ordered;
> > > +
> > > +	aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> > > +	hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> > > +	dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> > > +	ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > 
> > The trouble is, 'ordered' is still an unused variable on !DEBUG builds,
> > since the only user of ordered is that ASSERT.  So either we #ifdef
> > DEBUG the variable out of existence or employ one of those silly
> > 'ordered = ordered' constructions to shut up gcc, if that even still
> > works.
> > 
> 
> The warning goes away for me if we separate the initialization of
> ordered from the declaration. Do you observe otherwise?

Various versions of gcc will throw set-but-unused warnings on
this. Just #define it away or factor the debug code into another
function.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: linux-next: build warning after merge of the xfs tree

[Darrick J. Wong]

On Thu, Aug 31, 2017 at 11:22:20AM -0400, Brian Foster wrote:
> On Thu, Aug 31, 2017 at 07:57:52AM -0700, Darrick J. Wong wrote:
> > On Thu, Aug 31, 2017 at 06:30:41AM -0400, Brian Foster wrote:
> > > On Thu, Aug 31, 2017 at 10:07:03AM +1000, Stephen Rothwell wrote:
> > > > Hi all,
> > > > 
> > > > After merging the xfs tree, today's linux-next build (powerpc
> > > > ppc64_defconfig) produced this warning:
> > > > 
> > > > fs/xfs/xfs_buf_item.c: In function 'xfs_buf_item_unlock':
> > > > fs/xfs/xfs_buf_item.c:573:9: warning: unused variable 'ordered' [-Wunused-variable]
> > > >   bool   ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > > >          ^
> > > > 
> > > > Introduced by commit
> > > > 
> > > >   a097077ef708 ("xfs: remove unnecessary dirty bli format check for ordered bufs")
> > > > 
> > > 
> > > Ugh, this is due to the refactoring of this patch between v1 and v2. I
> > > specifically recall testing for this in v1 because I added the ordered
> > > bool purely to clean up the ASSERT(), then I apparently lost of track of
> > > it for v2.
> > > 
> > > Anyways.. Christoph, Darrick, preferences to clean this up..? I have no
> > > preference between the v1 or v2 factoring. Or if it's easier, we could
> > > always just drop something like the hunk below on top. Thoughts?
> > > 
> > > Brian
> > > 
> > > --- 8< ---
> > > 
> > > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > > index ef2c137..f5d25f5 100644
> > > --- a/fs/xfs/xfs_buf_item.c
> > > +++ b/fs/xfs/xfs_buf_item.c
> > > @@ -567,10 +567,15 @@ xfs_buf_item_unlock(
> > >  {
> > >  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
> > >  	struct xfs_buf		*bp = bip->bli_buf;
> > > -	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> > > -	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> > > -	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> > > -	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > > +	bool			aborted;
> > > +	bool			hold;
> > > +	bool			dirty;
> > > +	bool			ordered;
> > > +
> > > +	aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> > > +	hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> > > +	dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> > > +	ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > 
> > The trouble is, 'ordered' is still an unused variable on !DEBUG builds,
> > since the only user of ordered is that ASSERT.  So either we #ifdef
> > DEBUG the variable out of existence or employ one of those silly
> > 'ordered = ordered' constructions to shut up gcc, if that even still
> > works.
> > 
> 
> The warning goes away for me if we separate the initialization of
> ordered from the declaration. Do you observe otherwise?

Hm.  Seems to shut up gcc, so I guess it's fine.  In the past it would
whine, but I guess they fixed it or something.

Want to send it as a real [PATCH]?

--D

> 
> Brian
> 
> > --D
> > 
> > >  
> > >  	/* Clear the buffer's association with this transaction. */
> > >  	bp->b_transp = NULL;
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linux-next: build warning after merge of the xfs tree

[Brian Foster]

On Thu, Aug 31, 2017 at 07:57:52AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 31, 2017 at 06:30:41AM -0400, Brian Foster wrote:
> > On Thu, Aug 31, 2017 at 10:07:03AM +1000, Stephen Rothwell wrote:
> > > Hi all,
> > > 
> > > After merging the xfs tree, today's linux-next build (powerpc
> > > ppc64_defconfig) produced this warning:
> > > 
> > > fs/xfs/xfs_buf_item.c: In function 'xfs_buf_item_unlock':
> > > fs/xfs/xfs_buf_item.c:573:9: warning: unused variable 'ordered' [-Wunused-variable]
> > >   bool   ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > >          ^
> > > 
> > > Introduced by commit
> > > 
> > >   a097077ef708 ("xfs: remove unnecessary dirty bli format check for ordered bufs")
> > > 
> > 
> > Ugh, this is due to the refactoring of this patch between v1 and v2. I
> > specifically recall testing for this in v1 because I added the ordered
> > bool purely to clean up the ASSERT(), then I apparently lost of track of
> > it for v2.
> > 
> > Anyways.. Christoph, Darrick, preferences to clean this up..? I have no
> > preference between the v1 or v2 factoring. Or if it's easier, we could
> > always just drop something like the hunk below on top. Thoughts?
> > 
> > Brian
> > 
> > --- 8< ---
> > 
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index ef2c137..f5d25f5 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -567,10 +567,15 @@ xfs_buf_item_unlock(
> >  {
> >  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
> >  	struct xfs_buf		*bp = bip->bli_buf;
> > -	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> > -	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> > -	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> > -	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> > +	bool			aborted;
> > +	bool			hold;
> > +	bool			dirty;
> > +	bool			ordered;
> > +
> > +	aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> > +	hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> > +	dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> > +	ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> 
> The trouble is, 'ordered' is still an unused variable on !DEBUG builds,
> since the only user of ordered is that ASSERT.  So either we #ifdef
> DEBUG the variable out of existence or employ one of those silly
> 'ordered = ordered' constructions to shut up gcc, if that even still
> works.
> 

The warning goes away for me if we separate the initialization of
ordered from the declaration. Do you observe otherwise?

Brian

> --D
> 
> >  
> >  	/* Clear the buffer's association with this transaction. */
> >  	bp->b_transp = NULL;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linux-next: build warning after merge of the xfs tree

[Darrick J. Wong]

On Thu, Aug 31, 2017 at 06:30:41AM -0400, Brian Foster wrote:
> On Thu, Aug 31, 2017 at 10:07:03AM +1000, Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the xfs tree, today's linux-next build (powerpc
> > ppc64_defconfig) produced this warning:
> > 
> > fs/xfs/xfs_buf_item.c: In function 'xfs_buf_item_unlock':
> > fs/xfs/xfs_buf_item.c:573:9: warning: unused variable 'ordered' [-Wunused-variable]
> >   bool   ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> >          ^
> > 
> > Introduced by commit
> > 
> >   a097077ef708 ("xfs: remove unnecessary dirty bli format check for ordered bufs")
> > 
> 
> Ugh, this is due to the refactoring of this patch between v1 and v2. I
> specifically recall testing for this in v1 because I added the ordered
> bool purely to clean up the ASSERT(), then I apparently lost of track of
> it for v2.
> 
> Anyways.. Christoph, Darrick, preferences to clean this up..? I have no
> preference between the v1 or v2 factoring. Or if it's easier, we could
> always just drop something like the hunk below on top. Thoughts?
> 
> Brian
> 
> --- 8< ---
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index ef2c137..f5d25f5 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -567,10 +567,15 @@ xfs_buf_item_unlock(
>  {
>  	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
>  	struct xfs_buf		*bp = bip->bli_buf;
> -	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> -	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> -	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> -	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
> +	bool			aborted;
> +	bool			hold;
> +	bool			dirty;
> +	bool			ordered;
> +
> +	aborted = !!(lip->li_flags & XFS_LI_ABORTED);
> +	hold = !!(bip->bli_flags & XFS_BLI_HOLD);
> +	dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
> +	ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);

The trouble is, 'ordered' is still an unused variable on !DEBUG builds,
since the only user of ordered is that ASSERT.  So either we #ifdef
DEBUG the variable out of existence or employ one of those silly
'ordered = ordered' constructions to shut up gcc, if that even still
works.

--D

>  
>  	/* Clear the buffer's association with this transaction. */
>  	bp->b_transp = NULL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: linux-next: build warning after merge of the xfs tree

[Brian Foster]

On Thu, Aug 31, 2017 at 10:07:03AM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the xfs tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
> 
> fs/xfs/xfs_buf_item.c: In function 'xfs_buf_item_unlock':
> fs/xfs/xfs_buf_item.c:573:9: warning: unused variable 'ordered' [-Wunused-variable]
>   bool   ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
>          ^
> 
> Introduced by commit
> 
>   a097077ef708 ("xfs: remove unnecessary dirty bli format check for ordered bufs")
> 

Ugh, this is due to the refactoring of this patch between v1 and v2. I
specifically recall testing for this in v1 because I added the ordered
bool purely to clean up the ASSERT(), then I apparently lost of track of
it for v2.

Anyways.. Christoph, Darrick, preferences to clean this up..? I have no
preference between the v1 or v2 factoring. Or if it's easier, we could
always just drop something like the hunk below on top. Thoughts?

Brian

--- 8< ---

diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index ef2c137..f5d25f5 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -567,10 +567,15 @@ xfs_buf_item_unlock(
 {
 	struct xfs_buf_log_item	*bip = BUF_ITEM(lip);
 	struct xfs_buf		*bp = bip->bli_buf;
-	bool			aborted = !!(lip->li_flags & XFS_LI_ABORTED);
-	bool			hold = !!(bip->bli_flags & XFS_BLI_HOLD);
-	bool			dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
-	bool			ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
+	bool			aborted;
+	bool			hold;
+	bool			dirty;
+	bool			ordered;
+
+	aborted = !!(lip->li_flags & XFS_LI_ABORTED);
+	hold = !!(bip->bli_flags & XFS_BLI_HOLD);
+	dirty = !!(bip->bli_flags & XFS_BLI_DIRTY);
+	ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
 
 	/* Clear the buffer's association with this transaction. */
 	bp->b_transp = NULL;

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

Hi all,

After merging the xfs tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:

fs/xfs/xfs_buf_item.c: In function 'xfs_buf_item_unlock':
fs/xfs/xfs_buf_item.c:573:9: warning: unused variable 'ordered' [-Wunused-variable]
  bool   ordered = !!(bip->bli_flags & XFS_BLI_ORDERED);
         ^

Introduced by commit

  a097077ef708 ("xfs: remove unnecessary dirty bli format check for ordered bufs")

-- 
Cheers,
Stephen Rothwell

linux-next: build warning after merge of the xfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 689 bytes --]

Hi Dave,

After merging the xfs tree, today's linux-next build (x86_64 allmodconfig)
produced this warning:

fs/xfs/linux-2.6/xfs_aops.c: In function 'xfs_end_io':
fs/xfs/linux-2.6/xfs_aops.c:232: warning: 'error' may be used uninitialized in this function

Introduced by commit 77d7a0c2eeb285c9069e15396703d0cb9690ac50 ("xfs:
Non-blocking inode locking in IO completion").

I can't tell if this is a false positive.  If the first two "if"
statement bodies are skipped, then error is tested uninitialised.  It is
possible that at least one of them has to be executed.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]