ext4_da_write_end() and copied == 0

ext4_da_write_end() and copied == 0

[Andrea Arcangeli]

Hi,

I've been triggering ext4 troubles out of some experimental autonuma
code if I set knumad frequency rates very high.

The first patch looks fairly safe, but the second patch I've no idea
if it's correct and if damages stuff instead of fixing it, but clearly
it fixes the -EINVAL problem for me and it looks like to work fine now.

[PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()
[PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

If you make alternate fixes for this let me know and I can test
them. I can reproduce both issues instantly.

Thanks,
Andrea

[PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()

[Andrea Arcangeli]

If the pte mapping in generic_perform_write() is unmapped between
iov_iter_fault_in_readable() and iov_iter_copy_from_user_atomic(), the
"copied" parameter to ->end_write can be zero. ext4 couldn't cope with
it with delayed allocations enabled. This skips the i_disksize
enlargement logic if copied is zero and no new data was appeneded to
the inode.

 gdb> bt
 #0  0xffffffff811afe80 in ext4_da_should_update_i_disksize (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x1\
 08000, len=0x1000, copied=0x0, page=0xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2467
 #1  ext4_da_write_end (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x108000, len=0x1000, copied=0x0, page=0\
 xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2512
 #2  0xffffffff810d97f1 in generic_perform_write (iocb=<value optimized out>, iov=<value optimized out>, nr_segs=<value o\
 ptimized out>, pos=0x108000, ppos=0xffff88001e26be40, count=<value optimized out>, written=0x0) at mm/filemap.c:2440
 #3  generic_file_buffered_write (iocb=<value optimized out>, iov=<value optimized out>, nr_segs=<value optimized out>, p\
 os=0x108000, ppos=0xffff88001e26be40, count=<value optimized out>, written=0x0) at mm/filemap.c:2482
 #4  0xffffffff810db5d1 in __generic_file_aio_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=0x1, ppos=0\
 xffff88001e26be40) at mm/filemap.c:2600
 #5  0xffffffff810db853 in generic_file_aio_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=<value optimi\
 zed out>, pos=<value optimized out>) at mm/filemap.c:2632
 #6  0xffffffff811a71aa in ext4_file_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=0x1, pos=0x108000) a\
 t fs/ext4/file.c:136
 #7  0xffffffff811375aa in do_sync_write (filp=0xffff88003f606a80, buf=<value optimized out>, len=<value optimized out>, \
 ppos=0xffff88001e26bf48) at fs/read_write.c:406
 #8  0xffffffff81137e56 in vfs_write (file=0xffff88003f606a80, buf=0x1ec2960 <Address 0x1ec2960 out of bounds>, count=0x4\
 000, pos=0xffff88001e26bf48) at fs/read_write.c:435
 #9  0xffffffff8113816c in sys_write (fd=<value optimized out>, buf=0x1ec2960 <Address 0x1ec2960 out of bounds>, count=0x\
 4000) at fs/read_write.c:487
 #10 <signal handler called>
 #11 0x00007f120077a390 in __brk_reservation_fn_dmi_alloc__ ()
 #12 0x0000000000000000 in ?? ()
 gdb> print offset
 $22 = 0xffffffffffffffff
 gdb> print idx
 $23 = 0xffffffff
 gdb> print inode->i_blkbits
 $24 = 0xc
 gdb> up
 #1  ext4_da_write_end (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x108000, len=0x1000, copied=0x0, page=0\
 xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2512
 2512                    if (ext4_da_should_update_i_disksize(page, end)) {
 gdb> print start
 $25 = 0x0
 gdb> print end
 $26 = 0xffffffffffffffff
 gdb> print pos
 $27 = 0x108000
 gdb> print new_i_size
 $28 = 0x108000
 gdb> print ((struct ext4_inode_info *)((char *)inode-((int)(&((struct ext4_inode_info *)0)->vfs_inode))))->i_disksize
 $29 = 0xd9000
 gdb> down
 2467            for (i = 0; i < idx; i++)
 gdb> print i
 $30 = 0xd44acbee

This is 100% reproducible with some autonuma development code tuned in
a very aggressive manner (not normal way even for knumad) which does
"exotic" changes to the ptes. It wouldn't normally trigger but I don't
see why it can't happen normally if the page is added to swap cache in
between the two faults leading to "copied" being zero (which then
hangs in ext4). So it should be fixed. Especially possible with lumpy
reclaim (albeit disabled if compaction is enabled) as that would
ignore the young bits in the ptes.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 fs/ext4/inode.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fffec40..63f9541 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2508,7 +2508,7 @@ static int ext4_da_write_end(struct file *file,
 	 */
 
 	new_i_size = pos + copied;
-	if (new_i_size > EXT4_I(inode)->i_disksize) {
+	if (copied && new_i_size > EXT4_I(inode)->i_disksize) {
 		if (ext4_da_should_update_i_disksize(page, end)) {
 			down_write(&EXT4_I(inode)->i_data_sem);
 			if (new_i_size > EXT4_I(inode)->i_disksize) {

Re: [PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()

[Yongqiang Yang]

Hi Andrea,

I can not figure out why ext4 hangs.  Could you explain more on hanging?

Thanks,
Yongqiang.

On Mon, Dec 12, 2011 at 10:27 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> If the pte mapping in generic_perform_write() is unmapped between
> iov_iter_fault_in_readable() and iov_iter_copy_from_user_atomic(), the
> "copied" parameter to ->end_write can be zero. ext4 couldn't cope with
> it with delayed allocations enabled. This skips the i_disksize
> enlargement logic if copied is zero and no new data was appeneded to
> the inode.
>
>  gdb> bt
>  #0  0xffffffff811afe80 in ext4_da_should_update_i_disksize (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x1\
>  08000, len=0x1000, copied=0x0, page=0xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2467
>  #1  ext4_da_write_end (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x108000, len=0x1000, copied=0x0, page=0\
>  xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2512
>  #2  0xffffffff810d97f1 in generic_perform_write (iocb=<value optimized out>, iov=<value optimized out>, nr_segs=<value o\
>  ptimized out>, pos=0x108000, ppos=0xffff88001e26be40, count=<value optimized out>, written=0x0) at mm/filemap.c:2440
>  #3  generic_file_buffered_write (iocb=<value optimized out>, iov=<value optimized out>, nr_segs=<value optimized out>, p\
>  os=0x108000, ppos=0xffff88001e26be40, count=<value optimized out>, written=0x0) at mm/filemap.c:2482
>  #4  0xffffffff810db5d1 in __generic_file_aio_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=0x1, ppos=0\
>  xffff88001e26be40) at mm/filemap.c:2600
>  #5  0xffffffff810db853 in generic_file_aio_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=<value optimi\
>  zed out>, pos=<value optimized out>) at mm/filemap.c:2632
>  #6  0xffffffff811a71aa in ext4_file_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=0x1, pos=0x108000) a\
>  t fs/ext4/file.c:136
>  #7  0xffffffff811375aa in do_sync_write (filp=0xffff88003f606a80, buf=<value optimized out>, len=<value optimized out>, \
>  ppos=0xffff88001e26bf48) at fs/read_write.c:406
>  #8  0xffffffff81137e56 in vfs_write (file=0xffff88003f606a80, buf=0x1ec2960 <Address 0x1ec2960 out of bounds>, count=0x4\
>  000, pos=0xffff88001e26bf48) at fs/read_write.c:435
>  #9  0xffffffff8113816c in sys_write (fd=<value optimized out>, buf=0x1ec2960 <Address 0x1ec2960 out of bounds>, count=0x\
>  4000) at fs/read_write.c:487
>  #10 <signal handler called>
>  #11 0x00007f120077a390 in __brk_reservation_fn_dmi_alloc__ ()
>  #12 0x0000000000000000 in ?? ()
>  gdb> print offset
>  $22 = 0xffffffffffffffff
>  gdb> print idx
>  $23 = 0xffffffff
>  gdb> print inode->i_blkbits
>  $24 = 0xc
>  gdb> up
>  #1  ext4_da_write_end (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x108000, len=0x1000, copied=0x0, page=0\
>  xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2512
>  2512                    if (ext4_da_should_update_i_disksize(page, end)) {
>  gdb> print start
>  $25 = 0x0
>  gdb> print end
>  $26 = 0xffffffffffffffff
>  gdb> print pos
>  $27 = 0x108000
>  gdb> print new_i_size
>  $28 = 0x108000
>  gdb> print ((struct ext4_inode_info *)((char *)inode-((int)(&((struct ext4_inode_info *)0)->vfs_inode))))->i_disksize
>  $29 = 0xd9000
>  gdb> down
>  2467            for (i = 0; i < idx; i++)
>  gdb> print i
>  $30 = 0xd44acbee
>
> This is 100% reproducible with some autonuma development code tuned in
> a very aggressive manner (not normal way even for knumad) which does
> "exotic" changes to the ptes. It wouldn't normally trigger but I don't
> see why it can't happen normally if the page is added to swap cache in
> between the two faults leading to "copied" being zero (which then
> hangs in ext4). So it should be fixed. Especially possible with lumpy
> reclaim (albeit disabled if compaction is enabled) as that would
> ignore the young bits in the ptes.
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  fs/ext4/inode.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index fffec40..63f9541 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2508,7 +2508,7 @@ static int ext4_da_write_end(struct file *file,
>         */
>
>        new_i_size = pos + copied;
> -       if (new_i_size > EXT4_I(inode)->i_disksize) {
> +       if (copied && new_i_size > EXT4_I(inode)->i_disksize) {
>                if (ext4_da_should_update_i_disksize(page, end)) {
>                        down_write(&EXT4_I(inode)->i_data_sem);
>                        if (new_i_size > EXT4_I(inode)->i_disksize) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()

[Andrea Arcangeli]

On Mon, Dec 12, 2011 at 11:28:21AM +0800, Yongqiang Yang wrote:
> Hi Andrea,
> 
> I can not figure out why ext4 hangs.  Could you explain more on hanging?

Well I tried to explain it in the changeset comment.

If "copied" is zero, end/offset/idx becomes -1, and this loops:

	for (i = 0; i < idx; i++)

definitely a bug and checking ext4 git I don't see anything fixing it.

Re: [PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()

[Yongqiang Yang]

On Tue, Dec 13, 2011 at 12:57 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> On Mon, Dec 12, 2011 at 11:28:21AM +0800, Yongqiang Yang wrote:
>> Hi Andrea,
>>
>> I can not figure out why ext4 hangs.  Could you explain more on hanging?
>
> Well I tried to explain it in the changeset comment.
>
> If "copied" is zero, end/offset/idx becomes -1, and this loops:
>
>        for (i = 0; i < idx; i++)
>
> definitely a bug and checking ext4 git I don't see anything fixing it.
Got it.  Thanks.

Looks good to me.


-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()

[Jan Kara]

On Mon 12-12-11 03:27:07, Andrea Arcangeli wrote:
> If the pte mapping in generic_perform_write() is unmapped between
> iov_iter_fault_in_readable() and iov_iter_copy_from_user_atomic(), the
> "copied" parameter to ->end_write can be zero. ext4 couldn't cope with
> it with delayed allocations enabled.
  I'd expand this sentence with: because ext4_da_should_update_i_disksize()
is passed -1 as an offset and loops (almost) infinitely.
  I know it's also in the gdb dump below but it takes a while to notice it
if you don't know what you are looking for.

> This skips the i_disksize
> enlargement logic if copied is zero and no new data was appeneded to
> the inode.
> 
>  gdb> bt
>  #0  0xffffffff811afe80 in ext4_da_should_update_i_disksize (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x1\
>  08000, len=0x1000, copied=0x0, page=0xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2467
>  #1  ext4_da_write_end (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x108000, len=0x1000, copied=0x0, page=0\
>  xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2512
>  #2  0xffffffff810d97f1 in generic_perform_write (iocb=<value optimized out>, iov=<value optimized out>, nr_segs=<value o\
>  ptimized out>, pos=0x108000, ppos=0xffff88001e26be40, count=<value optimized out>, written=0x0) at mm/filemap.c:2440
>  #3  generic_file_buffered_write (iocb=<value optimized out>, iov=<value optimized out>, nr_segs=<value optimized out>, p\
>  os=0x108000, ppos=0xffff88001e26be40, count=<value optimized out>, written=0x0) at mm/filemap.c:2482
>  #4  0xffffffff810db5d1 in __generic_file_aio_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=0x1, ppos=0\
>  xffff88001e26be40) at mm/filemap.c:2600
>  #5  0xffffffff810db853 in generic_file_aio_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=<value optimi\
>  zed out>, pos=<value optimized out>) at mm/filemap.c:2632
>  #6  0xffffffff811a71aa in ext4_file_write (iocb=0xffff88001e26bde8, iov=0xffff88001e26bec8, nr_segs=0x1, pos=0x108000) a\
>  t fs/ext4/file.c:136
>  #7  0xffffffff811375aa in do_sync_write (filp=0xffff88003f606a80, buf=<value optimized out>, len=<value optimized out>, \
>  ppos=0xffff88001e26bf48) at fs/read_write.c:406
>  #8  0xffffffff81137e56 in vfs_write (file=0xffff88003f606a80, buf=0x1ec2960 <Address 0x1ec2960 out of bounds>, count=0x4\
>  000, pos=0xffff88001e26bf48) at fs/read_write.c:435
>  #9  0xffffffff8113816c in sys_write (fd=<value optimized out>, buf=0x1ec2960 <Address 0x1ec2960 out of bounds>, count=0x\
>  4000) at fs/read_write.c:487
>  #10 <signal handler called>
>  #11 0x00007f120077a390 in __brk_reservation_fn_dmi_alloc__ ()
>  #12 0x0000000000000000 in ?? ()
>  gdb> print offset
>  $22 = 0xffffffffffffffff
>  gdb> print idx
>  $23 = 0xffffffff
>  gdb> print inode->i_blkbits
>  $24 = 0xc
>  gdb> up
>  #1  ext4_da_write_end (file=0xffff88003f606a80, mapping=0xffff88001d3824e0, pos=0x108000, len=0x1000, copied=0x0, page=0\
>  xffffea0000d792e8, fsdata=0x0) at fs/ext4/inode.c:2512
>  2512                    if (ext4_da_should_update_i_disksize(page, end)) {
>  gdb> print start
>  $25 = 0x0
>  gdb> print end
>  $26 = 0xffffffffffffffff
>  gdb> print pos
>  $27 = 0x108000
>  gdb> print new_i_size
>  $28 = 0x108000
>  gdb> print ((struct ext4_inode_info *)((char *)inode-((int)(&((struct ext4_inode_info *)0)->vfs_inode))))->i_disksize
>  $29 = 0xd9000
>  gdb> down
>  2467            for (i = 0; i < idx; i++)
>  gdb> print i
>  $30 = 0xd44acbee
> 
> This is 100% reproducible with some autonuma development code tuned in
> a very aggressive manner (not normal way even for knumad) which does
> "exotic" changes to the ptes. It wouldn't normally trigger but I don't
> see why it can't happen normally if the page is added to swap cache in
> between the two faults leading to "copied" being zero (which then
> hangs in ext4). So it should be fixed. Especially possible with lumpy
> reclaim (albeit disabled if compaction is enabled) as that would
> ignore the young bits in the ptes.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
  You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/ext4/inode.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index fffec40..63f9541 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2508,7 +2508,7 @@ static int ext4_da_write_end(struct file *file,
>  	 */
>  
>  	new_i_size = pos + copied;
> -	if (new_i_size > EXT4_I(inode)->i_disksize) {
> +	if (copied && new_i_size > EXT4_I(inode)->i_disksize) {
>  		if (ext4_da_should_update_i_disksize(page, end)) {
>  			down_write(&EXT4_I(inode)->i_data_sem);
>  			if (new_i_size > EXT4_I(inode)->i_disksize) {
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/2] ext4: avoid hangs in ext4_da_should_update_i_disksize()

[Ted Ts'o]

On Mon, Dec 12, 2011 at 03:27:07AM +0100, Andrea Arcangeli wrote:
> If the pte mapping in generic_perform_write() is unmapped between
> iov_iter_fault_in_readable() and iov_iter_copy_from_user_atomic(), the
> "copied" parameter to ->end_write can be zero. ext4 couldn't cope with
> it with delayed allocations enabled. This skips the i_disksize
> enlargement logic if copied is zero and no new data was appeneded to
> the inode.
      ...
> 
> This is 100% reproducible with some autonuma development code tuned in
> a very aggressive manner (not normal way even for knumad) which does
> "exotic" changes to the ptes. It wouldn't normally trigger but I don't
> see why it can't happen normally if the page is added to swap cache in
> between the two faults leading to "copied" being zero (which then
> hangs in ext4). So it should be fixed. Especially possible with lumpy
> reclaim (albeit disabled if compaction is enabled) as that would
> ignore the young bits in the ptes.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>

Thanks, applied.

						- Ted

[PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

[Andrea Arcangeli]

If "copied" is zero (it can happen if the pte is unmapped before the
atomic copy_user that does the data copy runs) the "from" passed to
ext4_discard_partial_page_buffers_no_lock() points to pos-1, which
would correspond to a logical page index before the page->index
leading to ext4_discard_partial_page_buffers_no_lock() returning
-EINVAL (because index != page->index). In such a case write() returns
-EINVAL and userland gets a failure and filemap.c doesn't retry the
copy_user anymore.

I'm not certain of why exactly
ext4_discard_partial_page_buffers_no_lock() is run here, so it's hard
to tell if this is the correct fix. But it that functions clears data
starting from the "from" parameter, so regardless of the -EINVAL
retval, the right "from" to start clearing data should be pos+copied,
not pos+copied-1. If this assumption is correct, it could mean that
this bug in addition to the -EINVAL error, could also zero out 1 byte
by mistake. I'm not sure what the implications for that are (not sure
if data corruption is possible in some circumstances because of
that). I guess normally this functions runs on unmapped buffers and
the EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED makes it a noop on those.

After fixing the hang in ext4_da_should_update_i_disksize, this write
-EINVAL error becomes trivially reproducible with experimental knumad
autonuma code running at heavy frequency (not the normal case). But
like for the ext4_da_should_update_i_disksize hang, it should not be
impossible to reproduce it with legacy swapping behavior.

After dropping all caches a md5sum is successful and I can't find
errors anymore with this patch, the -EINVAL stops, but it's not
conclusive (and I haven't run e2fsck -f yet but I doubt this affects
metadata coherency, seems more like a delayalloc data issue).

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 fs/ext4/inode.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 63f9541..528c4c5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2534,11 +2534,11 @@ static int ext4_da_write_end(struct file *file,
 							page, fsdata);
 
 	page_len = PAGE_CACHE_SIZE -
-			((pos + copied - 1) & (PAGE_CACHE_SIZE - 1));
+			((pos + copied) & (PAGE_CACHE_SIZE - 1));
 
-	if (page_len > 0) {
+	if (page_len < PAGE_CACHE_SIZE) {
 		ret = ext4_discard_partial_page_buffers_no_lock(handle,
-			inode, page, pos + copied - 1, page_len,
+			inode, page, pos + copied, page_len,
 			EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED);
 	}
 

Re: [PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

[Yongqiang Yang]

Hi Andrea,

The code you are testing are removed in recent patches,  the patches
have not been merged.

Please try following patches:
[PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize
[PATCH 2/2] ext4: let ext4_discard_partial_buffers handle pages
without buffers correctly

and

[PATCH 1/2] ext4: remove a wrong BUG_ON in ext4_ext_convert_to_initialized
[PATCH 2/2] ext4: let ext4_bio_write_page handle EOF correctly

Yongqiang.

On Mon, Dec 12, 2011 at 10:27 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> If "copied" is zero (it can happen if the pte is unmapped before the
> atomic copy_user that does the data copy runs) the "from" passed to
> ext4_discard_partial_page_buffers_no_lock() points to pos-1, which
> would correspond to a logical page index before the page->index
> leading to ext4_discard_partial_page_buffers_no_lock() returning
> -EINVAL (because index != page->index). In such a case write() returns
> -EINVAL and userland gets a failure and filemap.c doesn't retry the
> copy_user anymore.
>
> I'm not certain of why exactly
> ext4_discard_partial_page_buffers_no_lock() is run here, so it's hard
> to tell if this is the correct fix. But it that functions clears data
> starting from the "from" parameter, so regardless of the -EINVAL
> retval, the right "from" to start clearing data should be pos+copied,
> not pos+copied-1. If this assumption is correct, it could mean that
> this bug in addition to the -EINVAL error, could also zero out 1 byte
> by mistake. I'm not sure what the implications for that are (not sure
> if data corruption is possible in some circumstances because of
> that). I guess normally this functions runs on unmapped buffers and
> the EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED makes it a noop on those.
>
> After fixing the hang in ext4_da_should_update_i_disksize, this write
> -EINVAL error becomes trivially reproducible with experimental knumad
> autonuma code running at heavy frequency (not the normal case). But
> like for the ext4_da_should_update_i_disksize hang, it should not be
> impossible to reproduce it with legacy swapping behavior.
>
> After dropping all caches a md5sum is successful and I can't find
> errors anymore with this patch, the -EINVAL stops, but it's not
> conclusive (and I haven't run e2fsck -f yet but I doubt this affects
> metadata coherency, seems more like a delayalloc data issue).
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  fs/ext4/inode.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 63f9541..528c4c5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2534,11 +2534,11 @@ static int ext4_da_write_end(struct file *file,
>                                                        page, fsdata);
>
>        page_len = PAGE_CACHE_SIZE -
> -                       ((pos + copied - 1) & (PAGE_CACHE_SIZE - 1));
> +                       ((pos + copied) & (PAGE_CACHE_SIZE - 1));
>
> -       if (page_len > 0) {
> +       if (page_len < PAGE_CACHE_SIZE) {
>                ret = ext4_discard_partial_page_buffers_no_lock(handle,
> -                       inode, page, pos + copied - 1, page_len,
> +                       inode, page, pos + copied, page_len,
>                        EXT4_DISCARD_PARTIAL_PG_ZERO_UNMAPPED);
>        }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

[Andrea Arcangeli]

On Mon, Dec 12, 2011 at 11:17:54AM +0800, Yongqiang Yang wrote:
> Hi Andrea,
> 
> The code you are testing are removed in recent patches,  the patches
> have not been merged.

I checked out the git ext4 tree and the two patches applies clean to
it. So I'm unsure why you say it was removed. origin/dev origin/master
are equal too.

> 
> Please try following patches:
> [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize < pagesize
> [PATCH 2/2] ext4: let ext4_discard_partial_buffers handle pages
> without buffers correctly
> 
> and
> 
> [PATCH 1/2] ext4: remove a wrong BUG_ON in ext4_ext_convert_to_initialized
> [PATCH 2/2] ext4: let ext4_bio_write_page handle EOF correctly

Is there a git tree with these patches? I'm not subscribed to the list.

Thanks,
Andrea

Re: [PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

[Allison Henderson]

On 12/12/2011 09:55 AM, Andrea Arcangeli wrote:
> On Mon, Dec 12, 2011 at 11:17:54AM +0800, Yongqiang Yang wrote:
>> Hi Andrea,
>>
>> The code you are testing are removed in recent patches,  the patches
>> have not been merged.
>
> I checked out the git ext4 tree and the two patches applies clean to
> it. So I'm unsure why you say it was removed. origin/dev origin/master
> are equal too.
>
>>
>> Please try following patches:
>> [PATCH 1/2] ext4: let mpage_submit_io works well when blocksize<  pagesize
>> [PATCH 2/2] ext4: let ext4_discard_partial_buffers handle pages
>> without buffers correctly
>>
>> and
>>
>> [PATCH 1/2] ext4: remove a wrong BUG_ON in ext4_ext_convert_to_initialized
>> [PATCH 2/2] ext4: let ext4_bio_write_page handle EOF correctly
>
> Is there a git tree with these patches? I'm not subscribed to the list.
>
> Thanks,
> Andrea

Hi Andrea,

I think what Yongqiang means is that the code you are modifying is being 
removed in the patches listed above and replaced with a different 
solution.  The patches have not yet been merged so you will not see them 
in a git tree yet, so you will need to apply them yourself.  If you are 
not subscribed to the list, you can still find the patches here:

http://www.spinics.net/lists/linux-ext4/msg29109.html
http://www.spinics.net/lists/linux-ext4/msg29110.html

http://www.spinics.net/lists/linux-ext4/msg29375.html
http://www.spinics.net/lists/linux-ext4/msg29376.html

We want to make sure the solution works for everyone, so please apply 
these patches and let us know if it corrects the issues you are seeing. 
  Thx!

Allison Henderson

> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

[Andrea Arcangeli]

Hi Allison,

On Mon, Dec 12, 2011 at 01:07:47PM -0700, Allison Henderson wrote:
> Hi Andrea,
> 
> I think what Yongqiang means is that the code you are modifying is being 
> removed in the patches listed above and replaced with a different 
> solution.  The patches have not yet been merged so you will not see them 
> in a git tree yet, so you will need to apply them yourself.  If you are 
> not subscribed to the list, you can still find the patches here:
> 
> http://www.spinics.net/lists/linux-ext4/msg29109.html
> http://www.spinics.net/lists/linux-ext4/msg29110.html
> 
> http://www.spinics.net/lists/linux-ext4/msg29375.html
> http://www.spinics.net/lists/linux-ext4/msg29376.html

Thanks for the links. Applied all 4.

> We want to make sure the solution works for everyone, so please apply 
> these patches and let us know if it corrects the issues you are seeing. 
>   Thx!

With all 4 patches applied, my 1/2 patch is still needed to avoid the
hang and writes hangs all the time (but now they don't return -EINVAL
anymore).

My patch 2/2 is not needed with the above 4 patches applied so that
can be discared if you apply the above 4 patches (but I suggest you
check my patch 2/2 too because if it happens to be correct it could
mean there's the potential of 1 byte data corruption in current
upstream kernels in some circumstances, in addition to the -EINVAL
failure in write()).

Let me know if you need me to test me anything else in relation to the
copied=0 case.

Thanks,
Andrea

Re: [PATCH 2/2] ext4: ext4_discard_partial_page_buffers_no_lock() wrong parameters

[Yongqiang Yang]

On Tue, Dec 13, 2011 at 8:40 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> Hi Allison,
>
> On Mon, Dec 12, 2011 at 01:07:47PM -0700, Allison Henderson wrote:
>> Hi Andrea,
>>
>> I think what Yongqiang means is that the code you are modifying is being
>> removed in the patches listed above and replaced with a different
>> solution.  The patches have not yet been merged so you will not see them
>> in a git tree yet, so you will need to apply them yourself.  If you are
>> not subscribed to the list, you can still find the patches here:
>>
>> http://www.spinics.net/lists/linux-ext4/msg29109.html
>> http://www.spinics.net/lists/linux-ext4/msg29110.html
>>
>> http://www.spinics.net/lists/linux-ext4/msg29375.html
>> http://www.spinics.net/lists/linux-ext4/msg29376.html
>
> Thanks for the links. Applied all 4.
>
>> We want to make sure the solution works for everyone, so please apply
>> these patches and let us know if it corrects the issues you are seeing.
>>   Thx!
>
> With all 4 patches applied, my 1/2 patch is still needed to avoid the
> hang and writes hangs all the time (but now they don't return -EINVAL
> anymore).
>
> My patch 2/2 is not needed with the above 4 patches applied so that
> can be discared if you apply the above 4 patches (but I suggest you
> check my patch 2/2 too because if it happens to be correct it could
> mean there's the potential of 1 byte data corruption in current
> upstream kernels in some circumstances, in addition to the -EINVAL
> failure in write()).
page_len is removed in the 2nd patch, so there is no problem related
to it any more.

Hi Andy and Hugh,

Andrea found the reason of EINVAL,  and the 2nd patch we have tested
can fix it.  Many thanks to Andrea.


Yongqiang.
>
> Let me know if you need me to test me anything else in relation to the
> copied=0 case.
>
> Thanks,
> Andrea



-- 
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html