On Sat, Aug 19, 2017 at 10:33:00AM +1000, Dave Chinner wrote:

On Fri, Aug 18, 2017 at 11:45:11AM -0700, Darrick J. Wong wrote:

quoted

On Fri, Aug 18, 2017 at 10:06:07AM -0700, Darrick J. Wong wrote:

quoted

On Fri, Aug 18, 2017 at 12:05:16AM -0700, Christoph Hellwig wrote:

quoted

On Thu, Aug 17, 2017 at 04:31:29PM -0700, Darrick J. Wong wrote:

quoted

Hi all,

This RFC combines all the random little fixes and improvements to the
verifiers that we've been talking about for the past month or so into a
single patch series!

We start by refactoring the long format btree block header verifier into
a single helper functionn and de-macroing dir block verifiers to make
them less shouty.  Next, we change verifier functions to return the
approximate instruction pointer of the faulting test so that we can
report more precise fault information to dmesg/tracepoints.

Just jumping here quickly because I don't have time for a detailed
review:

How good does this instruction pointer thing resolved to the actual
issue?

Ugh, it's terrible once you turn on the optimizer.

        if (!xfs_sb_version_hascrc(&mp->m_sb))                                  
                return __this_address;                                          
        if (!uuid_equal(&block->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))        
                return __this_address;                                          
        if (block->bb_u.s.bb_blkno != cpu_to_be64(bp->b_bn))                    
                return __this_address;                                          
        if (pag && be32_to_cpu(block->bb_u.s.bb_owner) != pag->pag_agno)        
                return __this_address;                                          
        return NULL;                                                            

becomes:

        if (!xfs_sb_version_hascrc(&mp->m_sb))                                  
                goto out;                                          
        if (!uuid_equal(&block->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))        
                goto out;                                          
        if (block->bb_u.s.bb_blkno != cpu_to_be64(bp->b_bn))                    
                goto out;                                          
        if (pag && be32_to_cpu(block->bb_u.s.bb_owner) != pag->pag_agno)        
                goto out;                                          
        return NULL;                                                            
out:
	return __this_address;

...which is totally worthless, unless we want to compile all the verifier
functions with __attribute__((optimize("O0"))), which is bogus.

<sigh> Back to the drawing board on that one.

Ok, there's /slightly/ less awful way to prevent gcc from optimizing the
verifier function to the point of imprecise pointer value, but it involves
writing to a volatile int:

/* stupidly prevent gcc from over-optimizing getting the instruction ptr */
extern volatile int xfs_lineno;
#define __this_address ({ __label__ __here; __here: xfs_lineno = __LINE__; &&__here; })

<grumble> Yucky, but it more or less works.

Can you declare the label as volatile, like you can an asm
statement to prevent the compiler from optimising out asm
statements?

Even so, given the yuckiness is very isolated and should only affect
the slow path code, I can live with this.

Hmmm.  I can't declare the label as volatile, but I /can/ inject
asm volatile("") and that seems to prevent gcc from moving code hunks
around:

#define __this_address	({ __label__ __here; __here: asm volatile(""); &&__here; })

--D

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
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