Re: [LTP] [PATCH v2] fallocate04: another check if SEEK_HOLE is not supported

[Jan Stancek <jstancek@redhat.com>]

----- Original Message -----
> From: "Zeng Linggang" <zenglg.jy@cn.fujitsu.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: "Alexey Kodanev" <alexey.kodanev@oracle.com>, "vasily isaenko" <vasily.isaenko@oracle.com>,
> ltp-list@lists.sourceforge.net
> Sent: Wednesday, 20 May, 2015 3:47:22 AM
> Subject: Re: [LTP] [PATCH v2] fallocate04: another check if SEEK_HOLE is not supported
> 
> Hi!
> 
> On Tue, 2015-05-19 at 07:18 -0400, Jan Stancek wrote:
> > 
> > 
> > 
> > ----- Original Message -----
> > > From: "Zeng Linggang" <zenglg.jy@cn.fujitsu.com>
> > > To: "Alexey Kodanev" <alexey.kodanev@oracle.com>
> > > Cc: "vasily isaenko" <vasily.isaenko@oracle.com>,
> > > ltp-list@lists.sourceforge.net
> > > Sent: Tuesday, 19 May, 2015 10:50:22 AM
> > > Subject: [LTP] [PATCH v2] fallocate04: another check if SEEK_HOLE is not
> > > 	supported
> > > 
> > > SEEK_HOLE is only supported since version 3.1. Check the specified
> > > range blocks are zeroed while the kernel does not supported SEEK_HOLE.
> > > 
> > > Signed-off-by: Zhang Jin <jy_zhangjin@cn.fujitsu.com>
> > > ---
> > >  testcases/kernel/syscalls/fallocate/fallocate04.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/testcases/kernel/syscalls/fallocate/fallocate04.c
> > > b/testcases/kernel/syscalls/fallocate/fallocate04.c
> > > index 911bbe8..e94c572 100644
> > > --- a/testcases/kernel/syscalls/fallocate/fallocate04.c
> > > +++ b/testcases/kernel/syscalls/fallocate/fallocate04.c
> > > @@ -98,13 +98,13 @@ static void setup(void)
> > >  	get_blocksize();
> > >  }
> > >  
> > > -static void check_file_data(const char exp_buf[], size_t size)
> > > +static void check_file_data_(const char exp_buf[], size_t size, off_t
> > > offset)
> > >  {
> > >  	char rbuf[size];
> > >  
> > >  	tst_resm(TINFO, "reading the file, compare with expected buffer");
> > >  
> > > -	SAFE_LSEEK(cleanup, fd, 0, SEEK_SET);
> > > +	SAFE_LSEEK(cleanup, fd, offset, SEEK_SET);
> > >  	SAFE_READ(cleanup, 1, fd, rbuf, size);
> > >  
> > >  	if (memcmp(exp_buf, rbuf, size)) {
> > > @@ -116,6 +116,11 @@ static void check_file_data(const char exp_buf[],
> > > size_t
> > > size)
> > >  	}
> > >  }
> > >  
> > > +static inline void check_file_data(const char exp_buf[], size_t size)
> > > +{
> > > +	check_file_data_(exp_buf, size, 0);
> > > +}
> > > +
> > >  static void test01(void)
> > >  {
> > >  	tst_resm(TINFO, "allocate '%zu' bytes", buf_size);
> > > @@ -158,7 +163,9 @@ static void test02(void)
> > >  			tst_brkm(TFAIL | TERRNO, cleanup,
> > >  				 "fallocate() or lseek() failed");
> > >  		}
> > > -		tst_resm(TWARN | TERRNO, "lseek() doesn't support SEEK_HOLE");
> > > +		char zeros[block_size];
> > > +		memset(zeros, 0, block_size);
> > > +		check_file_data_(zeros, block_size, block_size);
> > 
> > Hi,
> > 
> > Isn't this redundant?
> 
> To be honest, I do not think it is redundant.

Can you explain why?

> 
> > 
> > Couple lines below is this check, which also checks that range
> > <block_size, block_size*2> is zeroed.
> 
> Yep, this looks simple.
> This maybe cost more runtime because of 'fill_tst_buf'.
> But I do not reject it.

That code is already there, your patch is not removing it.

> 
> > 
> >         char exp_buf[buf_size];
> > 
> >         fill_tst_buf(exp_buf);
> >         memset(exp_buf + block_size, 0, block_size);
> >         check_file_data(exp_buf, buf_size);
> > 
> > Wouldn't it be enough to turn that warning into TINFO, for kernels < 3.1?
> > 
> > diff --git a/testcases/kernel/syscalls/fallocate/fallocate04.c
> > b/testcases/kernel/syscalls/fallocate/fallocate04.c
> > index 911bbe8..45d9827 100644
> > --- a/testcases/kernel/syscalls/fallocate/fallocate04.c
> > +++ b/testcases/kernel/syscalls/fallocate/fallocate04.c
> > @@ -158,9 +158,12 @@ static void test02(void)
> >                         tst_brkm(TFAIL | TERRNO, cleanup,
> >                                  "fallocate() or lseek() failed");
> >                 }
> > -               tst_resm(TWARN | TERRNO, "lseek() doesn't support
> > SEEK_HOLE");
> > +               if (tst_kvercmp(3, 1, 0) < 0)
> 
> How about put them before lseek(SEEK_HOLE) ?

I think we should do the check on all kernels. lseek() can only check that the
hole exists, but current check also verifies that hole has correct size and
content.

Regards,
Jan

> 
> if (tst_kvercmp(3, 1, 0) < 0) {
> 	fill_tst_buf();
> 	memset();
> 	check_file_data();
> } else {
> 	lseek(SEEK_HOLE);
> 	...
> }
> 	
> 
> Thank you very much.
> 
> Best regards,
> Zeng
> 
> > +                       tst_resm(TINFO, "lseek() doesn't support SEEK_HOLE,
> > "
> > +                               "this is expected for < 3.1 kernels");
> > +       } else {
> > +               tst_resm(TINFO, "found a hole at '%ld' offset", ret);
> >         }
> > -       tst_resm(TINFO, "found a hole at '%ld' offset", ret);
> >  
> >         size_t alloc_size1 = get_allocsize();
> >  
> > Regards,
> > Jan
> 
> 
> 

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list