ocfs2: avoid direct write if we fall back to buffered

PATCH New Dong Yang Li 3 participants 15 messages Apr 10, 2010 – Apr 23, 2010 OCFS2

lore

Share & tools

lore

Dong Yang Li

author

2010-04-10 07:37 UTC #0 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

I still get a bug with this check and without my patch:

[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1

the call trace is the same.

the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now,
so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.

IMHO, we can add this check back and fix this check, or we don't try to do direct write if we decided we can't in ocfs2_file_aio_write, after calling ocfs2_prepare_inode_for_write as my patch said.

Comments? ;-)

Br,
Li Dongyang

Quoted 1 line

Quoted 1 line
Quoted 1 line
Sunil Mushran 04/10/10 1:42 AM >>>

Quoted 45 lines · Li Dongyang

Quoted 23 lines · On Friday 09 April 2010 11:32:10 Tao Ma wrote:
Hi Dongyang,
Quoted 20 lines · Li Dongyang
Hi, Tao,
Quoted 17 lines · On Friday 09 April 2010 10:38:33 Tao Ma wrote:
Hi Dongyang,
Quoted 9 lines · Li Dongyang
This is because ocfs2_file_aio_write calls ocfs2_prepare_inode_for_write which sets direct_io to 0 if it finds out that direct IO would extend the file. But later we call __generic_file_aio_write which end's up calling generic_file_direct_write because the file has O_DIRECT flag.So every time we do a direct write extending the file, the inode->i_size gets inconsistent with the i_size on disk because we call generic_file_direct_write, and if we do a truncate after this, we will meet a bug in ocfs2_truncate_file.
yes we have O_DIRECT flag set and in __generic_file_aio_write it will call generic_file_direct_write first and then trigger to ocfs2_direct_IO. In this function we will check again and return 0. And _generic_file_aio_write will fall back to buffered write if the directIO can't write. Am I wrong somehow?
Quoted 6 lines
Quoted 4 lines
yes ocfs2_direct_IO has some check, but it just check if we are appending(the i_size <= offset), if the offset < i_size and offset + count > i_size, it will do direct io anyway. seems we also can fix this by adding a check to ocfs2_direct_IO.
It is done by ocfs2_direct_IO_get_blocks. Just debug the kernel and you will get what I mean. ;)
Do you mean this section in ocfs2_direct_IO_get_blocks:? /*
* Any write past EOF is not allowed because we'd be extending. */
if (create && (iblock + max_blocks) > inode_blocks) {
    ret = -EIO;
    goto bail;
}
I was using the linus tree
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
and we don't have that check, but I can find this in the
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git, introduced by commit 564f8a3228879d6962edb3432d01bcd7499a67ec

Quoted 3 lines

and now with this check I got what you mean, you are right, but I wonder why the linus tree doesn't have this check? and are we suppose to do with this? IMHO we can just push this commit to linus tree.

commit 5fe878ae7f82fbf0830dbfaee4c5ca18f3aee442

Author: Christoph Hellwig 
Date:   Tue Dec 15 16:47:50 2009 -0800

direct-io: cleanup blockdev_direct_IO locking

This check was removed recently by the above patch.

Joel Becker

2010-04-10 09:37 UTC #1 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 6 lines · Dong Yang Li

I still get a bug with this check and without my patch:

[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1

the call trace is the same.

What's the testcase?

Joel

Signature · 7 lines

	f/8 and be there.

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Li Dongyang

author

2010-04-10 09:48 UTC #2 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 4 lines · On Saturday 10 April 2010 17:37:18 Joel Becker wrote:

Quoted 3 lines · Dong Yang Li
I still get a bug with this check and without my patch:

Quoted 9 lines

Quoted 4 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression:
le64_to_cpu(fe->i_size) != i_size_read(inode) [16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.
What's the testcase?
Joel

Just fire up "fsstress -d <ocfs2 mount point> -l 1000 -p 1000 -n 1000", fsstress is a tool from ltp package.
JJ first found it on a 3-nodes cluster with user stack, I also can reproduce it on a 2-nodes cluster with o2cb.

Li Dongyang

Tao Ma

2010-04-12 05:16 UTC #3 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

In reply to Dong Yang Li

Hi dong yang,

Quoted 1 line · Dong Yang Li

I still get a bug with this check and without my patch:

yes, the check doesn't work actually in this case.

Quoted 9 lines

[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1
the call trace is the same.
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now,
so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.

yeah, you are right.

Quoted 3 lines

IMHO, we can add this check back and fix this check, or we don't try to do direct write if we decided we can't in ocfs2_file_aio_write, after calling ocfs2_prepare_inode_for_write as my patch said.

I think we only need to check this condition in get_blocks. So would you mind providing a patch? You old method is too aggressive actually.

btw, I have created a small test script which will expose this bug easily. So you don't need to use the time-consuming fsstress test now. Just use it to test your fix.

echo 'y'|mkfs.ocfs2 --fs-features=local,noinline-data -b 4K -C 4K $DEVICE 1000000
mount -t ocfs2 $DEVICE $MNT_DIR
echo "foo" > $MNT_DIR/foo
dd if=/dev/zero of=$MNT_DIR/foo bs=4K count=1 conv=notrunc oflag=direct echo "foo" > $MNT_DIR/foo
# The kernel should panic here.

Regards,
Tao

Quoted 67 lines

Comments? ;-)
Br,
Li Dongyang
Quoted 1 line
Quoted 1 line
Quoted 1 line
Sunil Mushran 04/10/10 1:42 AM >>>
Quoted 45 lines · Li Dongyang
Quoted 23 lines · On Friday 09 April 2010 11:32:10 Tao Ma wrote:
Hi Dongyang,
Quoted 20 lines · Li Dongyang
Hi, Tao,
Quoted 17 lines · On Friday 09 April 2010 10:38:33 Tao Ma wrote:
Hi Dongyang,
Quoted 9 lines · Li Dongyang
This is because ocfs2_file_aio_write calls ocfs2_prepare_inode_for_write which sets direct_io to 0 if it finds out that direct IO would extend the file. But later we call __generic_file_aio_write which end's up calling generic_file_direct_write because the file has O_DIRECT flag.So every time we do a direct write extending the file, the inode->i_size gets inconsistent with the i_size on disk because we call generic_file_direct_write, and if we do a truncate after this, we will meet a bug in ocfs2_truncate_file.
yes we have O_DIRECT flag set and in __generic_file_aio_write it will call generic_file_direct_write first and then trigger to ocfs2_direct_IO. In this function we will check again and return 0. And _generic_file_aio_write will fall back to buffered write if the directIO can't write. Am I wrong somehow?
Quoted 6 lines
Quoted 4 lines
yes ocfs2_direct_IO has some check, but it just check if we are appending(the i_size <= offset), if the offset < i_size and offset + count > i_size, it will do direct io anyway. seems we also can fix this by adding a check to ocfs2_direct_IO.
It is done by ocfs2_direct_IO_get_blocks. Just debug the kernel and you will get what I mean. ;)
Do you mean this section in ocfs2_direct_IO_get_blocks:? /*
* Any write past EOF is not allowed because we'd be extending. */
if (create && (iblock + max_blocks) > inode_blocks) {
    ret = -EIO;
    goto bail;
}
I was using the linus tree
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
and we don't have that check, but I can find this in the
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git, introduced by commit 564f8a3228879d6962edb3432d01bcd7499a67ec
Quoted 3 lines
and now with this check I got what you mean, you are right, but I wonder why the linus tree doesn't have this check? and are we suppose to do with this? IMHO we can just push this commit to linus tree.
commit 5fe878ae7f82fbf0830dbfaee4c5ca18f3aee442
Author: Christoph Hellwig 
Date:   Tue Dec 15 16:47:50 2009 -0800
direct-io: cleanup blockdev_direct_IO locking
This check was removed recently by the above patch.

Li Dongyang

author

2010-04-12 05:31 UTC #4 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Hi, Tao

Quoted 13 lines · On Monday 12 April 2010 13:16:43 Tao Ma wrote:

Hi dong yang,
Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 6 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression:
le64_to_cpu(fe->i_size) != i_size_read(inode) [16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.

Quoted 8 lines

Quoted 5 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now, so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.

Thanks for the script,
and a stupid question: why we still try to call __generic_file_aio_write and let it try direct write first in ocfs2_file_aio_write even we decided we could not do the direct write?

Quoted 7 lines

Quoted 3 lines
IMHO, we can add this check back and fix this check, or we don't try to do direct write if we decided we can't in ocfs2_file_aio_write, after calling ocfs2_prepare_inode_for_write as my patch said.
I think we only need to check this condition in get_blocks. So would you mind providing a patch? You old method is too aggressive actually.

what about add this check in ocfs2_direct_IO? if we see we are extending just return 0. right now we only check if we are appending.

Quoted 86 lines

btw, I have created a small test script which will expose this bug easily. So you don't need to use the time-consuming fsstress test now. Just use it to test your fix.
echo 'y'|mkfs.ocfs2 --fs-features=local,noinline-data -b 4K -C 4K $DEVICE 1000000
mount -t ocfs2 $DEVICE $MNT_DIR
echo "foo" > $MNT_DIR/foo
dd if=/dev/zero of=$MNT_DIR/foo bs=4K count=1 conv=notrunc oflag=direct echo "foo" > $MNT_DIR/foo
# The kernel should panic here.
Regards,
Tao
Quoted 70 lines
Comments? ;-)
Br,
Li Dongyang
Quoted 1 line
Quoted 1 line
Quoted 1 line
Sunil Mushran 04/10/10 1:42 AM >>>
Quoted 50 lines · Li Dongyang
Quoted 26 lines · On Friday 09 April 2010 11:32:10 Tao Ma wrote:
Hi Dongyang,
Quoted 23 lines · Li Dongyang
Hi, Tao,
Quoted 19 lines · On Friday 09 April 2010 10:38:33 Tao Ma wrote:
Hi Dongyang,
Quoted 10 lines · Li Dongyang
This is because ocfs2_file_aio_write calls ocfs2_prepare_inode_for_write which sets direct_io to 0 if it finds out that direct IO would extend the file. But later we call __generic_file_aio_write which end's up calling generic_file_direct_write because the file has O_DIRECT flag.So every time we do a direct write extending the file, the inode->i_size gets inconsistent with the i_size on disk because we call
generic_file_direct_write, and if we do a truncate after this, we will meet a bug in ocfs2_truncate_file.
yes we have O_DIRECT flag set and in __generic_file_aio_write it will call generic_file_direct_write first and then trigger to ocfs2_direct_IO. In this function we will check again and return 0. And _generic_file_aio_write will fall back to buffered write if the directIO can't write. Am I wrong somehow?
Quoted 7 lines
Quoted 4 lines
yes ocfs2_direct_IO has some check, but it just check if we are appending(the i_size <= offset), if the offset < i_size and offset + count > i_size, it will do direct io anyway. seems we also can fix this by adding a check to ocfs2_direct_IO.
It is done by ocfs2_direct_IO_get_blocks. Just debug the kernel and you will get what I mean. ;)
Do you mean this section in ocfs2_direct_IO_get_blocks:? /*
* Any write past EOF is not allowed because we'd be extending. */
if (create && (iblock + max_blocks) > inode_blocks) {
    ret = -EIO;
    goto bail;
}
I was using the linus tree
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
and we don't have that check, but I can find this in the
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git,
introduced by commit 564f8a3228879d6962edb3432d01bcd7499a67ec
Quoted 3 lines
and now with this check I got what you mean, you are right, but I wonder why the linus tree doesn't have this check? and are we suppose to do with this? IMHO we can just push this commit to linus tree.
commit 5fe878ae7f82fbf0830dbfaee4c5ca18f3aee442
Author: Christoph Hellwig
Date:   Tue Dec 15 16:47:50 2009 -0800
direct-io: cleanup blockdev_direct_IO locking
This check was removed recently by the above patch.

Tao Ma

2010-04-12 06:24 UTC #5 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 25 lines · Li Dongyang

Hi, Tao
Quoted 12 lines · On Monday 12 April 2010 13:16:43 Tao Ma wrote:
Hi dong yang,
Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 6 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression:
le64_to_cpu(fe->i_size) != i_size_read(inode) [16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.
Quoted 7 lines
Quoted 5 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now, so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.
Thanks for the script,
and a stupid question: why we still try to call __generic_file_aio_write and let it try direct write first in ocfs2_file_aio_write even we decided we could not do the direct write?

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
IMHO, we can add this check back and fix this check, or we don't try to do direct write if we decided we can't in ocfs2_file_aio_write, after calling ocfs2_prepare_inode_for_write as my patch said.
I think we only need to check this condition in get_blocks. So would you mind providing a patch? You old method is too aggressive actually.
what about add this check in ocfs2_direct_IO? if we see we are extending just return 0. right now we only check if we are appending.

As for the 2 questions, I just want to do buffered write as small as possible since it has to lock inode, create pages and then sync pages etc(you can check ocfs2_write_begin/end for details. ;) ). So say this question, actually only the last block needed to be buffered ioed and i_size get updated accordingly.

I just checked ext4_direct_IO and actually it updated the disk size at the end of direct_IO. So maybe we can work like that also.

Regards,
Tao

Quoted 79 lines

Quoted 79 lines
btw, I have created a small test script which will expose this bug easily. So you don't need to use the time-consuming fsstress test now. Just use it to test your fix.
echo 'y'|mkfs.ocfs2 --fs-features=local,noinline-data -b 4K -C 4K $DEVICE 1000000
mount -t ocfs2 $DEVICE $MNT_DIR
echo "foo" > $MNT_DIR/foo
dd if=/dev/zero of=$MNT_DIR/foo bs=4K count=1 conv=notrunc oflag=direct echo "foo" > $MNT_DIR/foo
# The kernel should panic here.
Regards,
Tao
Quoted 64 lines
Comments? ;-)
Br,
Li Dongyang
Quoted 1 line
Quoted 1 line
Quoted 1 line
Sunil Mushran 04/10/10 1:42 AM >>>
Quoted 46 lines · Li Dongyang
Quoted 24 lines · On Friday 09 April 2010 11:32:10 Tao Ma wrote:
Hi Dongyang,
Quoted 21 lines · Li Dongyang
Hi, Tao,
Quoted 18 lines · On Friday 09 April 2010 10:38:33 Tao Ma wrote:
Hi Dongyang,
Quoted 10 lines · Li Dongyang
This is because ocfs2_file_aio_write calls ocfs2_prepare_inode_for_write which sets direct_io to 0 if it finds out that direct IO would extend the file. But later we call __generic_file_aio_write which end's up calling generic_file_direct_write because the file has O_DIRECT flag.So every time we do a direct write extending the file, the inode->i_size gets inconsistent with the i_size on disk because we call
generic_file_direct_write, and if we do a truncate after this, we will meet a bug in ocfs2_truncate_file.
yes we have O_DIRECT flag set and in __generic_file_aio_write it will call generic_file_direct_write first and then trigger to ocfs2_direct_IO. In this function we will check again and return 0. And _generic_file_aio_write will fall back to buffered write if the directIO can't write. Am I wrong somehow?
Quoted 6 lines
Quoted 4 lines
yes ocfs2_direct_IO has some check, but it just check if we are appending(the i_size <= offset), if the offset < i_size and offset + count > i_size, it will do direct io anyway. seems we also can fix this by adding a check to ocfs2_direct_IO.
It is done by ocfs2_direct_IO_get_blocks. Just debug the kernel and you will get what I mean. ;)
Do you mean this section in ocfs2_direct_IO_get_blocks:? /*
* Any write past EOF is not allowed because we'd be extending. */
if (create && (iblock + max_blocks) > inode_blocks) {
    ret = -EIO;
    goto bail;
}
I was using the linus tree
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
and we don't have that check, but I can find this in the
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git,
introduced by commit 564f8a3228879d6962edb3432d01bcd7499a67ec
Quoted 3 lines
and now with this check I got what you mean, you are right, but I wonder why the linus tree doesn't have this check? and are we suppose to do with this? IMHO we can just push this commit to linus tree.
commit 5fe878ae7f82fbf0830dbfaee4c5ca18f3aee442
Author: Christoph Hellwig
Date:   Tue Dec 15 16:47:50 2009 -0800
direct-io: cleanup blockdev_direct_IO locking
This check was removed recently by the above patch.

Tao Ma

2010-04-14 02:44 UTC #6 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Hi Dongyang,

Quoted 35 lines · Tao Ma

Quoted 25 lines · Li Dongyang
Hi, Tao
Quoted 12 lines · On Monday 12 April 2010 13:16:43 Tao Ma wrote:
Hi dong yang,
Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 6 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression:
le64_to_cpu(fe->i_size) != i_size_read(inode) [16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.
Quoted 7 lines
Quoted 5 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now, so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.
Thanks for the script,
and a stupid question: why we still try to call __generic_file_aio_write and let it try direct write first in ocfs2_file_aio_write even we decided we could not do the direct write?
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
IMHO, we can add this check back and fix this check, or we don't try to do direct write if we decided we can't in ocfs2_file_aio_write, after calling ocfs2_prepare_inode_for_write as my patch said.
I think we only need to check this condition in get_blocks. So would you mind providing a patch? You old method is too aggressive actually.
what about add this check in ocfs2_direct_IO? if we see we are extending just return 0. right now we only check if we are appending.

Quoted 8 lines

As for the 2 questions, I just want to do buffered write as small as possible since it has to lock inode, create pages and then sync pages etc(you can check ocfs2_write_begin/end for details. ;) ). So say this question, actually only the last block needed to be buffered ioed and i_size get updated accordingly.
I just checked ext4_direct_IO and actually it updated the disk size at the end of direct_IO. So maybe we can work like that also.

sorry, I mislead you.
Joel pointed out that except the problem my little script exposed, there is another problem about ip_alloc_sem locking. So we have to fall back to buffer write from the very beginning. I just saw that Joel has commented your original patch, so do please revise it.

Regards,
Tao

Quoted 23 lines

Regards,
Tao
Quoted 20 lines
Quoted 20 lines
btw, I have created a small test script which will expose this bug easily. So you don't need to use the time-consuming fsstress test now. Just use it to test your fix.
echo 'y'|mkfs.ocfs2 --fs-features=local,noinline-data -b 4K -C 4K $DEVICE 1000000
mount -t ocfs2 $DEVICE $MNT_DIR
echo "foo" > $MNT_DIR/foo
dd if=/dev/zero of=$MNT_DIR/foo bs=4K count=1 conv=notrunc oflag=direct echo "foo" > $MNT_DIR/foo
# The kernel should panic here.
Regards,
Tao
Quoted 5 lines
Comments? ;-)
Br,
Li Dongyang

Li Dongyang

author

2010-04-14 05:47 UTC #7 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Hi, Tao

Quoted 34 lines · On Wednesday 14 April 2010 10:44:24 Tao Ma wrote:

Hi Dongyang,
Quoted 31 lines · Tao Ma
Quoted 30 lines · Li Dongyang
Hi, Tao
Quoted 15 lines · On Monday 12 April 2010 13:16:43 Tao Ma wrote:
Hi dong yang,
Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 8 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug
expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157]
(13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.
Quoted 7 lines
Quoted 5 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now, so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.
Thanks for the script,
and a stupid question: why we still try to call __generic_file_aio_write and let it try direct write first in ocfs2_file_aio_write even we decided we could not do the direct write?

yes, I also concerned about the i_alloc_sem, that's why I asked the question above. and I think we can remove the check in ocfs2_direct_IO_get_blocks, as it does not work. and your suggestion sb->s_blocksize * (iblocks+contig_blocks)>inode->i_size will give -EIO to those good direct writes which are not going beyond i_size but also played with the last partial block. e.g. an inode allocated with 4 blocks and i_size is 3 * 4096 + 2000 and we wanna do a direct io with pos=0 and length=3 * 4096 + 1000, as we are at block level in o_d_I_g_b().
in that case, we will fall back to buffered io and the i_alloc_sem have already down read in ocfs2_file_aio_write(), I wonder if that will cause a problem?

Quoted 17 lines

Quoted 17 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
IMHO, we can add this check back and fix this check, or we don't try to do direct write if we decided we can't in ocfs2_file_aio_write, after calling ocfs2_prepare_inode_for_write as my patch said.
I think we only need to check this condition in get_blocks. So would you mind providing a patch? You old method is too aggressive actually.
what about add this check in ocfs2_direct_IO? if we see we are extending just return 0. right now we only check if we are appending.
As for the 2 questions, I just want to do buffered write as small as possible since it has to lock inode, create pages and then sync pages etc(you can check ocfs2_write_begin/end for details. ;) ). So say this question, actually only the last block needed to be buffered ioed and i_size get updated accordingly.

Quoted 8 lines

Quoted 2 lines
I just checked ext4_direct_IO and actually it updated the disk size at the end of direct_IO. So maybe we can work like that also.
sorry, I mislead you.
Joel pointed out that except the problem my little script exposed, there is another problem about ip_alloc_sem locking. So we have to fall back to buffer write from the very beginning. I just saw that Joel has commented your original patch, so do please revise it.

working on that,
Br,
Li Dongyang

Quoted 28 lines

Regards,
Tao
Quoted 23 lines
Regards,
Tao
Quoted 20 lines
Quoted 20 lines
btw, I have created a small test script which will expose this bug easily. So you don't need to use the time-consuming fsstress test now. Just use it to test your fix.
echo 'y'|mkfs.ocfs2 --fs-features=local,noinline-data -b 4K -C 4K $DEVICE 1000000
mount -t ocfs2 $DEVICE $MNT_DIR
echo "foo" > $MNT_DIR/foo
dd if=/dev/zero of=$MNT_DIR/foo bs=4K count=1 conv=notrunc oflag=direct echo "foo" > $MNT_DIR/foo
# The kernel should panic here.
Regards,
Tao
Quoted 5 lines
Comments? ;-)
Br,
Li Dongyang

Tao Ma

2010-04-14 06:08 UTC #8 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Hi Dongyang,

Quoted 33 lines · Li Dongyang

Hi, Tao
Quoted 31 lines · On Wednesday 14 April 2010 10:44:24 Tao Ma wrote:
Hi Dongyang,
Quoted 28 lines · Tao Ma
Quoted 27 lines · Li Dongyang
Hi, Tao
Quoted 14 lines · On Monday 12 April 2010 13:16:43 Tao Ma wrote:
Hi dong yang,
Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 8 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug
expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157]
(13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.
Quoted 6 lines
Quoted 5 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now, so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.
Thanks for the script,
and a stupid question: why we still try to call __generic_file_aio_write and let it try direct write first in ocfs2_file_aio_write even we decided we could not do the direct write?

Quoted 7 lines

yes, I also concerned about the i_alloc_sem, that's why I asked the question above. and I think we can remove the check in ocfs2_direct_IO_get_blocks, as it does not work. and your suggestion sb->s_blocksize * (iblocks+contig_blocks)>inode->i_size will give -EIO to those good direct writes which are not going beyond i_size but also played with the last partial block. e.g. an inode allocated with 4 blocks and i_size is 3 * 4096 + 2000 and we wanna do a direct io with pos=0 and length=3 * 4096 + 1000, as we are at block level in o_d_I_g_b().

No, I don't mean to return -EIO. My old thought is that we can just clear_buffer_mapped like a hole and return 0, and then let buffer_write to do it. Maybe I said it somehow misleading.

Regards,
Tao

Joel Becker

2010-04-13 23:54 UTC #9 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

In reply to Tao Ma

Quoted 5 lines · Tao Ma

Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 2 lines

Quoted 8 lines

Quoted 7 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1
the call trace is the same.
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now,
so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.

I think Sunil and I have found the real culprit.
If a file is opened for O_DIRECT, and there are no holes,
refcounts or anything, we are doing direct I/O. ocfs2_file_aio_write() (o_f_a_w() from now on) locks things down like so: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock). We have ip_alloc_sem preventing size changes on the local node and rw_lock preventing size changes on other nodes. We call generic_file_direct_write() ourselves.
If a file is not opened with O_DIRECT, we are doing regular
buffered writes. o_f_a_w() locks like so: lock(i_mutex), EX(rw_lock). It is protecting against other nodes, but it does not touch ip_alloc_sem. Why? Because we call __generic_file_aio_write(), which will call ->write_begin(). ip_alloc_sem will be taken inside ->write_begin(). That's where we protect against other local processes.
You may already see where I'm going with this. If we are open
with O_DIRECT, but we have to fall back to buffered, we will do this
locking: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock), NL(rw_lock), up_read(ip_alloc_sem), EX(rw_lock). That is, we start with the direct I/O locking, then back off and do the buffered locking. But when we get into __g_f_a_w(), it will try the direct I/O again. If the leading portion of the I/O is capable of direct I/O, it will go into direct mode *without ever taking ip_alloc_sem*. Once it gets to the portion of the I/O that cannot be done direct, it will fall back to buffered for the rest of the I/O and will call ->write_begin() as expected.
So this I/O that extends i_size to the end of the allocation
will proceed as a direct I/O but will not have ip_alloc_sem. Thus truncate (and any other allocation change) can race on the local machine.
I think some form of Dong Yang's patch is going to be necessary.

Joel

Signature · 9 lines

Life's Little Instruction Book #306

	"Take a nap on Sunday afternoons."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Tao Ma

2010-04-14 00:13 UTC #10 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 40 lines · Joel Becker

Quoted 11 lines · Tao Ma
Quoted 2 lines · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 5 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression: le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1
the call trace is the same.
Quoted 5 lines
Quoted 3 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now,
so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.
I think Sunil and I have found the real culprit.
If a file is opened for O_DIRECT, and there are no holes,
refcounts or anything, we are doing direct I/O. ocfs2_file_aio_write() (o_f_a_w() from now on) locks things down like so: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock). We have ip_alloc_sem preventing size changes on the local node and rw_lock preventing size changes on other nodes. We call generic_file_direct_write() ourselves.
If a file is not opened with O_DIRECT, we are doing regular
buffered writes. o_f_a_w() locks like so: lock(i_mutex), EX(rw_lock). It is protecting against other nodes, but it does not touch ip_alloc_sem. Why? Because we call __generic_file_aio_write(), which will call ->write_begin(). ip_alloc_sem will be taken inside ->write_begin(). That's where we protect against other local processes.
You may already see where I'm going with this. If we are open
with O_DIRECT, but we have to fall back to buffered, we will do this
locking: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock), NL(rw_lock), up_read(ip_alloc_sem), EX(rw_lock). That is, we start with the direct I/O locking, then back off and do the buffered locking. But when we get into __g_f_a_w(), it will try the direct I/O again. If the leading portion of the I/O is capable of direct I/O, it will go into direct mode *without ever taking ip_alloc_sem*. Once it gets to the

Quoted 9 lines

portion of the I/O that cannot be done direct, it will fall back to buffered for the rest of the I/O and will call ->write_begin() as expected.
So this I/O that extends i_size to the end of the allocation
will proceed as a direct I/O but will not have ip_alloc_sem. Thus truncate (and any other allocation change) can race on the local machine.
I think some form of Dong Yang's patch is going to be necessary.

oh, yes, your analysis make sense.
But that doesn't prove that my get_block suggestion doesn't work in this case.
If we can find this situation in ocfs2_direct_IO_get_blocks and clear_buffer_mapped. It should fall
back to buffer_write for the last block and update i_size properly. Actually, the check should be easy.
sb->s_blocksize * (iblocks+contig_blocks)>inode->i_size.

In this way, we should have to fall to buffer write only necessarily.

Regards,
Tao

Quoted 3 lines

Joel

Li Dongyang

author

2010-04-14 05:58 UTC #11 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

In reply to Joel Becker

Hi, Joel

Quoted 41 lines · On Wednesday 14 April 2010 07:54:35 Joel Becker wrote:

Quoted 11 lines · Tao Ma
Quoted 1 line · Dong Yang Li
I still get a bug with this check and without my patch:
yes, the check doesn't work actually in this case.
Quoted 6 lines
[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression:
le64_to_cpu(fe->i_size) != i_size_read(inode) [16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode i_size = 811008 != di i_size = 809011, i_flags = 0x1 the call trace is the same.
Quoted 7 lines
Quoted 5 lines
the problem is this check in ocfs2_direct_IO_get_blocks just check if we are going beyond the blocks right now, so if a direct write won't play with new blocks but extending the i_size still get a pass, like the error above said, di->i_size is 809011, using 198 blocks and the direct write end up with i_size 811008, just same 198 blocks.
yeah, you are right.
I think Sunil and I have found the real culprit.
If a file is opened for O_DIRECT, and there are no holes,
refcounts or anything, we are doing direct I/O. ocfs2_file_aio_write() (o_f_a_w() from now on) locks things down like so: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock). We have ip_alloc_sem preventing size changes on the local node and rw_lock preventing size changes on other nodes. We call generic_file_direct_write() ourselves.
If a file is not opened with O_DIRECT, we are doing regular
buffered writes. o_f_a_w() locks like so: lock(i_mutex), EX(rw_lock). It is protecting against other nodes, but it does not touch ip_alloc_sem. Why? Because we call __generic_file_aio_write(), which will call ->write_begin(). ip_alloc_sem will be taken inside ->write_begin(). That's where we protect against other local processes.
You may already see where I'm going with this. If we are open
with O_DIRECT, but we have to fall back to buffered, we will do this
locking: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock), NL(rw_lock), up_read(ip_alloc_sem), EX(rw_lock). That is, we start with the direct I/O locking, then back off and do the buffered locking. But when we get into __g_f_a_w(), it will try the direct I/O again. If the leading portion of the I/O is capable of direct I/O, it will go into direct mode *without ever taking ip_alloc_sem*. Once it gets to the

Quoted 9 lines

portion of the I/O that cannot be done direct, it will fall back to buffered for the rest of the I/O and will call ->write_begin() as expected.
So this I/O that extends i_size to the end of the allocation
will proceed as a direct I/O but will not have ip_alloc_sem. Thus truncate (and any other allocation change) can race on the local machine.
I think some form of Dong Yang's patch is going to be necessary.

Thanks for the great explanation and analysis, but I only see we down write the OCFS2_I(inode)->ip_alloc_sem in ->write_begin() and we are taking inode->i_alloc_sem in o_f_a_w() when we try to do a direct write, not the ip_alloc_sem. Am I missing something?

Br,
Li Dongyang

Quoted 2 lines

Joel

Joel Becker

2010-04-14 19:20 UTC #12 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 31 lines · Li Dongyang

Quoted 21 lines · On Wednesday 14 April 2010 07:54:35 Joel Becker wrote:
I think Sunil and I have found the real culprit.
If a file is opened for O_DIRECT, and there are no holes,
refcounts or anything, we are doing direct I/O. ocfs2_file_aio_write() (o_f_a_w() from now on) locks things down like so: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock). We have ip_alloc_sem preventing size changes on the local node and rw_lock preventing size changes on other nodes. We call generic_file_direct_write() ourselves.
If a file is not opened with O_DIRECT, we are doing regular
buffered writes. o_f_a_w() locks like so: lock(i_mutex), EX(rw_lock). It is protecting against other nodes, but it does not touch ip_alloc_sem. Why? Because we call __generic_file_aio_write(), which will call ->write_begin(). ip_alloc_sem will be taken inside ->write_begin(). That's where we protect against other local processes.
You may already see where I'm going with this. If we are open
with O_DIRECT, but we have to fall back to buffered, we will do this
locking: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock), NL(rw_lock), up_read(ip_alloc_sem), EX(rw_lock). That is, we start with the direct I/O locking, then back off and do the buffered locking. But when we get into __g_f_a_w(), it will try the direct I/O again. If the leading portion of the I/O is capable of direct I/O, it will go into direct mode *without ever taking ip_alloc_sem*. Once it gets to the
Quoted 9 lines
portion of the I/O that cannot be done direct, it will fall back to buffered for the rest of the I/O and will call ->write_begin() as expected.
So this I/O that extends i_size to the end of the allocation
will proceed as a direct I/O but will not have ip_alloc_sem. Thus truncate (and any other allocation change) can race on the local machine.
I think some form of Dong Yang's patch is going to be necessary.

Quoted 4 lines

Thanks for the great explanation and analysis, but I only see we down write the OCFS2_I(inode)->ip_alloc_sem in ->write_begin() and we are taking inode->i_alloc_sem in o_f_a_w() when we try to do a direct write, not the ip_alloc_sem. Am I missing something?

You're right, we use i_alloc_sem in the direct case and
ip_alloc_sem in the buffered case. It is, however, for the same reason. i_alloc_sem is about competing with the VFS (eg, vs vfs_truncate()). ip_alloc_sem is about competing with ourselves (ocfs2_truncate(), ocfs2_readpage(), etc).
While I should be saying i_alloc_sem above for the direct I/O
case, the rest of the analysis is still correct. We need to be holding i_alloc_sem if we're going to be issuing direct I/Os, and we are not holding it in the fallback to buffered case.

Joel

Signature · 9 lines

"Depend on the rabbit's foot if you will, but remember, it didn't
 help the rabbit."
	- R. E. Shay

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Li Dongyang

author

2010-04-22 14:13 UTC #13 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 47 lines · On Thursday 15 April 2010 03:20:11 Joel Becker wrote:

Quoted 31 lines · Li Dongyang
Quoted 21 lines · On Wednesday 14 April 2010 07:54:35 Joel Becker wrote:
I think Sunil and I have found the real culprit.
If a file is opened for O_DIRECT, and there are no holes,
refcounts or anything, we are doing direct I/O. ocfs2_file_aio_write() (o_f_a_w() from now on) locks things down like so: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock). We have ip_alloc_sem preventing size changes on the local node and rw_lock preventing size changes on other nodes. We call generic_file_direct_write() ourselves.
If a file is not opened with O_DIRECT, we are doing regular
buffered writes. o_f_a_w() locks like so: lock(i_mutex), EX(rw_lock). It is protecting against other nodes, but it does not touch ip_alloc_sem. Why? Because we call __generic_file_aio_write(), which will call ->write_begin(). ip_alloc_sem will be taken inside ->write_begin(). That's where we protect against other local processes. You may already see where I'm going with this. If we are open with O_DIRECT, but we have to fall back to buffered, we will do this locking: lock(i_mutex), down_read(ip_alloc_sem), PR(rw_lock), NL(rw_lock), up_read(ip_alloc_sem), EX(rw_lock). That is, we start with the direct I/O locking, then back off and do the buffered locking.
But when we get into __g_f_a_w(), it will try the direct I/O again.
If the leading portion of the I/O is capable of direct I/O, it will go into direct mode *without ever taking ip_alloc_sem*. Once it gets to
Quoted 8 lines
the portion of the I/O that cannot be done direct, it will fall back to buffered for the rest of the I/O and will call ->write_begin() as expected.
So this I/O that extends i_size to the end of the allocation
will proceed as a direct I/O but will not have ip_alloc_sem. Thus truncate (and any other allocation change) can race on the local machine.
I think some form of Dong Yang's patch is going to be necessary.
Quoted 4 lines
Thanks for the great explanation and analysis, but I only see we down write the OCFS2_I(inode)->ip_alloc_sem in ->write_begin() and we are taking inode->i_alloc_sem in o_f_a_w() when we try to do a direct write, not the ip_alloc_sem. Am I missing something?
You're right, we use i_alloc_sem in the direct case and
ip_alloc_sem in the buffered case. It is, however, for the same reason. i_alloc_sem is about competing with the VFS (eg, vs vfs_truncate()). ip_alloc_sem is about competing with ourselves (ocfs2_truncate(), ocfs2_readpage(), etc).
While I should be saying i_alloc_sem above for the direct I/O
case, the rest of the analysis is still correct. We need to be holding i_alloc_sem if we're going to be issuing direct I/Os, and we are not holding it in the fallback to buffered case.

Quoted 2 lines

Joel

another question: why do we only take PR on the rw_lock and do not allow a direct write extending the i_size?
Br
Li Dongyang

Joel Becker

2010-04-23 20:06 UTC #14 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered

Quoted 2 lines · Li Dongyang

another question: why do we only take PR on the rw_lock and do not allow a direct write extending the i_size?

The rw_lock is the cluster equivalent of the alloc_sem. We take
a PR when we intend to use the existing allocation but will not modify the allocation. When we want to change the allocation (filling holes, changing unwritten extents, growing i_size) we must take an EX.

Joel

Signature · 9 lines

Life's Little Instruction Book #30

	"Never buy a house without a fireplace."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127