[PATCH] generic/446: make sure all background processes are dead before umount

[PATCH] generic/446: make sure all background processes are dead before umount

[Eryu Guan]

The $dread_pid refers to the while-true-do loop, wait for $dread_pid
doesn't mean the xfs_io direct read process is already dead,
sometimes xfs_io process is still running and blocking
_scratch_unmount.

Fix it by making the direct read does a fixed number of loop and
break out the second mmap-fpunch loop if the first loop exits. At
this point we're sure that there's no unfinished background process
blocking the umount.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 tests/generic/446 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/generic/446 b/tests/generic/446
index 62ae449de6b1..83ff4ac9edd3 100755
--- a/tests/generic/446
+++ b/tests/generic/446
@@ -61,20 +61,22 @@ filesz=$((65536 * 2))
 $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file >> $seqres.full
 
 # run a background dio read to a hole in a loop
-while true; do
+for i in `seq 0 999`; do
 	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file > /dev/null 2>&1
 done &
 
 dread_pid=$!
 
 # run mapped write to the same hole as dio read
-for i in `seq 0 999`; do
+while true; do
 	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
 		> /dev/null
 	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
+	if ! kill -s 0 $dread_pid >/dev/null 2>&1; then
+		break
+	fi
 done
 
-kill -9 $dread_pid > /dev/null 2>&1
 wait $dread_pid > /dev/null 2>&1
 
 echo "Silence is golden"
-- 
2.13.3

Re: [PATCH] generic/446: make sure all background processes are dead before umount

[Xiao Yang]

On 2017/07/24 18:11, Eryu Guan wrote:
> The $dread_pid refers to the while-true-do loop, wait for $dread_pid
> doesn't mean the xfs_io direct read process is already dead,
> sometimes xfs_io process is still running and blocking
> _scratch_unmount.
Hi Eryu,

Could you tell me what can i do to trigger this block?
> Fix it by making the direct read does a fixed number of loop and
> break out the second mmap-fpunch loop if the first loop exits. At

Does it break out the second mmap-fpunch loop if the first loop doesn't
exit?

> this point we're sure that there's no unfinished background process
> blocking the umount.

The others look fine.

Thanks,
Xiao Yang.
> Signed-off-by: Eryu Guan <eguan@redhat.com>
> ---
>  tests/generic/446 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tests/generic/446 b/tests/generic/446
> index 62ae449de6b1..83ff4ac9edd3 100755
> --- a/tests/generic/446
> +++ b/tests/generic/446
> @@ -61,20 +61,22 @@ filesz=$((65536 * 2))
>  $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file >> $seqres.full
>  
>  # run a background dio read to a hole in a loop
> -while true; do
> +for i in `seq 0 999`; do
>  	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file > /dev/null 2>&1
>  done &
>  
>  dread_pid=$!
>  
>  # run mapped write to the same hole as dio read
> -for i in `seq 0 999`; do
> +while true; do
>  	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
>  		> /dev/null
>  	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
> +	if ! kill -s 0 $dread_pid >/dev/null 2>&1; then
> +		break
> +	fi
>  done
>  
> -kill -9 $dread_pid > /dev/null 2>&1
>  wait $dread_pid > /dev/null 2>&1
>  
>  echo "Silence is golden"

Re: [PATCH] generic/446: make sure all background processes are dead before umount

[Eryu Guan]

On Tue, Jul 25, 2017 at 09:39:28AM +0800, Xiao Yang wrote:
> On 2017/07/24 18:11, Eryu Guan wrote:
> > The $dread_pid refers to the while-true-do loop, wait for $dread_pid
> > doesn't mean the xfs_io direct read process is already dead,
> > sometimes xfs_io process is still running and blocking
> > _scratch_unmount.
> Hi Eryu,
> 
> Could you tell me what can i do to trigger this block?

I don't have a reliable reproducer, I hit this in my automated tests,
perhaps easier to hit with slow disks. You can try replace the fpunch
operation with a pwrite, I can hit the block much easier this way.

> > Fix it by making the direct read does a fixed number of loop and
> > break out the second mmap-fpunch loop if the first loop exits. At
> 
> Does it break out the second mmap-fpunch loop if the first loop doesn't
> exit?

The second mmap-fpunch loop won't break until it finds the first loop
exits.

> 
> > this point we're sure that there's no unfinished background process
> > blocking the umount.
> 
> The others look fine.

Thanks!

Eryu

> 
> Thanks,
> Xiao Yang.
> > Signed-off-by: Eryu Guan <eguan@redhat.com>
> > ---
> >  tests/generic/446 | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/generic/446 b/tests/generic/446
> > index 62ae449de6b1..83ff4ac9edd3 100755
> > --- a/tests/generic/446
> > +++ b/tests/generic/446
> > @@ -61,20 +61,22 @@ filesz=$((65536 * 2))
> >  $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file >> $seqres.full
> >  
> >  # run a background dio read to a hole in a loop
> > -while true; do
> > +for i in `seq 0 999`; do
> >  	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file > /dev/null 2>&1
> >  done &
> >  
> >  dread_pid=$!
> >  
> >  # run mapped write to the same hole as dio read
> > -for i in `seq 0 999`; do
> > +while true; do
> >  	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
> >  		> /dev/null
> >  	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
> > +	if ! kill -s 0 $dread_pid >/dev/null 2>&1; then
> > +		break
> > +	fi
> >  done
> >  
> > -kill -9 $dread_pid > /dev/null 2>&1
> >  wait $dread_pid > /dev/null 2>&1
> >  
> >  echo "Silence is golden"
> 
> 
> 

Re: [PATCH] generic/446: make sure all background processes are dead before umount

[Xiao Yang]

On 2017/07/25 14:31, Eryu Guan wrote:
> On Tue, Jul 25, 2017 at 09:39:28AM +0800, Xiao Yang wrote:
>> On 2017/07/24 18:11, Eryu Guan wrote:
>>> The $dread_pid refers to the while-true-do loop, wait for $dread_pid
>>> doesn't mean the xfs_io direct read process is already dead,
>>> sometimes xfs_io process is still running and blocking
>>> _scratch_unmount.
>> Hi Eryu,
>>
>> Could you tell me what can i do to trigger this block?
> I don't have a reliable reproducer, I hit this in my automated tests,
> perhaps easier to hit with slow disks. You can try replace the fpunch
> operation with a pwrite, I can hit the block much easier this way.
Hi Eryu,

Thanks for your explanation.  I can hit this block easily when replacing 
the fpunch
operation with a pwrite.
>>> Fix it by making the direct read does a fixed number of loop and
>>> break out the second mmap-fpunch loop if the first loop exits. At
>> Does it break out the second mmap-fpunch loop if the first loop doesn't
>> exit?
> The second mmap-fpunch loop won't break until it finds the first loop
> exits.
got it. :-)
>>> this point we're sure that there's no unfinished background process
>>> blocking the umount.
>> The others look fine.
> Thanks!
>
> Eryu
>
>> Thanks,
>> Xiao Yang.
>>> Signed-off-by: Eryu Guan<eguan@redhat.com>
>>> ---
>>>   tests/generic/446 | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/generic/446 b/tests/generic/446
>>> index 62ae449de6b1..83ff4ac9edd3 100755
>>> --- a/tests/generic/446
>>> +++ b/tests/generic/446
>>> @@ -61,20 +61,22 @@ filesz=$((65536 * 2))
>>>   $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file>>  $seqres.full
>>>
>>>   # run a background dio read to a hole in a loop
>>> -while true; do
>>> +for i in `seq 0 999`; do
>>>   	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file>  /dev/null 2>&1
>>>   done&
>>>
>>>   dread_pid=$!
>>>
>>>   # run mapped write to the same hole as dio read
>>> -for i in `seq 0 999`; do
>>> +while true; do
>>>   	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
>>>   		>  /dev/null
>>>   	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file>  /dev/null
>>> +	if ! kill -s 0 $dread_pid>/dev/null 2>&1; then
>>> +		break
>>> +	fi
>>>   done
Could we simplify this break, as below:
while kill -s 0 $dread_pid >/dev/null 2>&1; do
     $XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" 
$SCRATCH_MNT/file \
 > /dev/null
     $XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
done

Thanks,
Xiao Yang.
>>>
>>> -kill -9 $dread_pid>  /dev/null 2>&1
>>>   wait $dread_pid>  /dev/null 2>&1
>>>
>>>   echo "Silence is golden"
>>
>>
>
> .
>

Re: [PATCH] generic/446: make sure all background processes are dead before umount

[Eryu Guan]

On Tue, Jul 25, 2017 at 03:43:59PM +0800, Xiao Yang wrote:
> > > >   # run mapped write to the same hole as dio read
> > > > -for i in `seq 0 999`; do
> > > > +while true; do
> > > >   	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
> > > >   		>  /dev/null
> > > >   	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file>  /dev/null
> > > > +	if ! kill -s 0 $dread_pid>/dev/null 2>&1; then
> > > > +		break
> > > > +	fi
> > > >   done
> Could we simplify this break, as below:
> while kill -s 0 $dread_pid >/dev/null 2>&1; do
>     $XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file
> \
> > /dev/null
>     $XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
> done

This looks better, thanks! I'll send v2.

Eryu

[PATCH v2] generic/446: make sure all background processes are dead before umount

[Eryu Guan]

The $dread_pid refers to the while-true-do loop, wait for $dread_pid
doesn't mean the xfs_io direct read process is already dead,
sometimes xfs_io process is still running and blocking
_scratch_unmount.

Fix it by making the direct read does a fixed number of loop and
break out the second mmap-fpunch loop if the first loop exits. At
this point we're sure that there's no unfinished background process
blocking the umount.

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
v2:
- move 'kill' to while loop condition to simplify the second loop
- add more comments about when to exit the loop

 tests/generic/446 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/generic/446 b/tests/generic/446
index 62ae449de6b1..6a1b69aa2edc 100755
--- a/tests/generic/446
+++ b/tests/generic/446
@@ -61,20 +61,20 @@ filesz=$((65536 * 2))
 $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file >> $seqres.full
 
 # run a background dio read to a hole in a loop
-while true; do
+for i in `seq 0 999`; do
 	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file > /dev/null 2>&1
 done &
 
 dread_pid=$!
 
 # run mapped write to the same hole as dio read
-for i in `seq 0 999`; do
+# loop until background dio read exits
+while kill -s 0 $dread_pid >/dev/null 2>&1; do
 	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
 		> /dev/null
 	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
 done
 
-kill -9 $dread_pid > /dev/null 2>&1
 wait $dread_pid > /dev/null 2>&1
 
 echo "Silence is golden"
-- 
2.13.3

Re: [PATCH v2] generic/446: make sure all background processes are dead before umount

[Eryu Guan]

Hi Xiao Yang,

On Tue, Jul 25, 2017 at 04:04:21PM +0800, Eryu Guan wrote:
> The $dread_pid refers to the while-true-do loop, wait for $dread_pid
> doesn't mean the xfs_io direct read process is already dead,
> sometimes xfs_io process is still running and blocking
> _scratch_unmount.
> 
> Fix it by making the direct read does a fixed number of loop and
> break out the second mmap-fpunch loop if the first loop exits. At
> this point we're sure that there's no unfinished background process
> blocking the umount.
> 
> Signed-off-by: Eryu Guan <eguan@redhat.com>
> ---
> v2:
> - move 'kill' to while loop condition to simplify the second loop
> - add more comments about when to exit the loop

Would you please help test & review and provide reviewed-by tag if you
think it's OK? Thanks a lot!

Eryu

> 
>  tests/generic/446 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/generic/446 b/tests/generic/446
> index 62ae449de6b1..6a1b69aa2edc 100755
> --- a/tests/generic/446
> +++ b/tests/generic/446
> @@ -61,20 +61,20 @@ filesz=$((65536 * 2))
>  $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file >> $seqres.full
>  
>  # run a background dio read to a hole in a loop
> -while true; do
> +for i in `seq 0 999`; do
>  	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file > /dev/null 2>&1
>  done &
>  
>  dread_pid=$!
>  
>  # run mapped write to the same hole as dio read
> -for i in `seq 0 999`; do
> +# loop until background dio read exits
> +while kill -s 0 $dread_pid >/dev/null 2>&1; do
>  	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
>  		> /dev/null
>  	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file > /dev/null
>  done
>  
> -kill -9 $dread_pid > /dev/null 2>&1
>  wait $dread_pid > /dev/null 2>&1
>  
>  echo "Silence is golden"
> -- 
> 2.13.3
> 

Re: [PATCH v2] generic/446: make sure all background processes are dead before umount

[Xiao Yang]

On 2017/07/27 13:22, Eryu Guan wrote:
> Hi Xiao Yang,
>
> On Tue, Jul 25, 2017 at 04:04:21PM +0800, Eryu Guan wrote:
>> The $dread_pid refers to the while-true-do loop, wait for $dread_pid
>> doesn't mean the xfs_io direct read process is already dead,
>> sometimes xfs_io process is still running and blocking
>> _scratch_unmount.
>>
>> Fix it by making the direct read does a fixed number of loop and
>> break out the second mmap-fpunch loop if the first loop exits. At
>> this point we're sure that there's no unfinished background process
>> blocking the umount.
>>
>> Signed-off-by: Eryu Guan<eguan@redhat.com>
>> ---
>> v2:
>> - move 'kill' to while loop condition to simplify the second loop
>> - add more comments about when to exit the loop
> Would you please help test&  review and provide reviewed-by tag if you
> think it's OK? Thanks a lot!
Hi Eryu,

I have tested it on my host,  so it's fine. :-)

Reviewed-by: Xiao Yang <yangx.jy@cn.fujitsu.com>

Thanks,
Xiao Yang
> Eryu
>
>>   tests/generic/446 | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/generic/446 b/tests/generic/446
>> index 62ae449de6b1..6a1b69aa2edc 100755
>> --- a/tests/generic/446
>> +++ b/tests/generic/446
>> @@ -61,20 +61,20 @@ filesz=$((65536 * 2))
>>   $XFS_IO_PROG -f -c "truncate $((filesz * 2))" $SCRATCH_MNT/file>>  $seqres.full
>>
>>   # run a background dio read to a hole in a loop
>> -while true; do
>> +for i in `seq 0 999`; do
>>   	$XFS_IO_PROG -d -c "pread 0 $filesz" $SCRATCH_MNT/file>  /dev/null 2>&1
>>   done&
>>
>>   dread_pid=$!
>>
>>   # run mapped write to the same hole as dio read
>> -for i in `seq 0 999`; do
>> +# loop until background dio read exits
>> +while kill -s 0 $dread_pid>/dev/null 2>&1; do
>>   	$XFS_IO_PROG -c "mmap 0 $filesz" -c "mwrite 0 $filesz" $SCRATCH_MNT/file \
>>   		>  /dev/null
>>   	$XFS_IO_PROG -c "fpunch 0 $filesz" $SCRATCH_MNT/file>  /dev/null
>>   done
>>
>> -kill -9 $dread_pid>  /dev/null 2>&1
>>   wait $dread_pid>  /dev/null 2>&1
>>
>>   echo "Silence is golden"
>> -- 
>> 2.13.3
>>
>
> .
>