* [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() @ 2013-03-18 4:48 Jeff Liu 2013-03-18 20:03 ` Michael L. Semon 2013-03-18 23:30 ` Dave Chinner 0 siblings, 2 replies; 7+ messages in thread From: Jeff Liu @ 2013-03-18 4:48 UTC (permalink / raw) To: xfs@oss.sgi.com; +Cc: Michael L. Semon Hello, Here is the v2 patch for fixing ASSERTION failed at xfs_vm_write_failed() according to Dave's comments, so I added Dave as SOB for credit. Hi Michael, Please kindly try at your at your convenience. Thanks, -Jeff From: Jie Liu <jeff.liu@oracle.com> In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK which is 0xfffff000 as an unsigned long, that is fine on 64-bit system no matter the request pos is 32-bit or 64-bit. However, on 32-bit system, the high 32-bit in it will be masked off with (pos & PAGE_MASK) for 64-bit pos request. As a result, the evaluated block_offset is incorrect which will trigger ASSERTION failure: ASSERT(block_offset + from == pos); In this case, we can get the following kernel Panic if the CONFIG_XFS_DEBUG is enabled: [ 68.700573] XFS: Assertion failed: block_offset + from == pos, file: fs/xfs/xfs_aops.c, line: 1504 [ 68.700656] ------------[ cut here ]------------ [ 68.700692] kernel BUG at fs/xfs/xfs_message.c:100! [ 68.700742] invalid opcode: 0000 [#1] SMP ........ [ 68.701678] Pid: 4057, comm: mkfs.xfs Tainted: G O 3.9.0-rc2 #1 [ 68.701722] EIP: 0060:[<f94a7e8b>] EFLAGS: 00010282 CPU: 0 [ 68.701783] EIP is at assfail+0x2b/0x30 [xfs] [ 68.701819] EAX: 00000056 EBX: f6ef28a0 ECX: 00000007 EDX: f57d22a4 [ 68.701852] ESI: 1c2fb000 EDI: 00000000 EBP: ea6b5d30 ESP: ea6b5d1c [ 68.701895] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 [ 68.701934] CR0: 8005003b CR2: 094f3ff4 CR3: 2bcb4000 CR4: 000006f0 [ 68.701970] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 [ 68.702011] DR6: ffff0ff0 DR7: 00000400 [ 68.702046] Process mkfs.xfs (pid: 4057, ti=ea6b4000 task=ea5799e0 task.ti=ea6b4000) [ 68.702086] Stack: [ 68.702124] 00000000 f9525c48 f951fa80 f951f96b 000005e4 ea6b5d7c f9494b34 c19b0ea2 [ 68.702445] 00000066 f3d6c620 c19b0ea2 00000000 e9a91458 00001000 00000000 00000000 [ 68.702868] 00000000 c15c7e89 00000000 1c2fb000 00000000 00000000 1c2fb000 00000080 [ 68.703192] Call Trace: [ 68.703248] [<f9494b34>] xfs_vm_write_failed+0x74/0x1b0 [xfs] [ 68.703441] [<c15c7e89>] ? printk+0x4d/0x4f [ 68.703496] [<f9494d7d>] xfs_vm_write_begin+0x10d/0x170 [xfs] [ 68.703535] [<c110a34c>] generic_file_buffered_write+0xdc/0x210 [ 68.703583] [<f949b669>] xfs_file_buffered_aio_write+0xf9/0x190 [xfs] [ 68.703629] [<f949b7f3>] xfs_file_aio_write+0xf3/0x160 [xfs] [ 68.703668] [<c115e504>] do_sync_write+0x94/0xd0 [ 68.703716] [<c115ed1f>] vfs_write+0x8f/0x160 [ 68.703753] [<c115e470>] ? wait_on_retry_sync_kiocb+0x50/0x50 [ 68.703794] [<c115f017>] sys_write+0x47/0x80 [ 68.703830] [<c15d860d>] sysenter_do_call+0x12/0x28 ............. [ 68.704203] EIP: [<f94a7e8b>] assfail+0x2b/0x30 [xfs] SS:ESP 0068:ea6b5d1c [ 68.706615] ---[ end trace cdd9af4f4ecab42f ]--- [ 68.706687] Kernel panic - not syncing: Fatal exception This patch fix the block_offset evaluation to clear the lower 12 bits as: block_offset = pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT Hence, the ASSERTION should be correct because the from offset in a page is evaluated to have the lower 12 bits only. Thanks Dave Chinner for pointing this out. Reported-by: Michael L. Semon <mlsemon35@gmail.com> Signed-off-by: Jie Liu <jeff.liu@oracle.com> Signed-off-by: Dave Chinner <david@fromorbit.com> --- fs/xfs/xfs_aops.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 5f707e5..a418e17 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -1494,7 +1494,8 @@ xfs_vm_write_failed( loff_t pos, unsigned len) { - loff_t block_offset = pos & PAGE_MASK; + loff_t block_offset = (pos >> PAGE_CACHE_SHIFT) << + PAGE_CACHE_SHIFT; loff_t block_start; loff_t block_end; loff_t from = pos & (PAGE_CACHE_SIZE - 1); -- 1.7.9.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() 2013-03-18 4:48 [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() Jeff Liu @ 2013-03-18 20:03 ` Michael L. Semon 2013-03-18 23:30 ` Dave Chinner 1 sibling, 0 replies; 7+ messages in thread From: Michael L. Semon @ 2013-03-18 20:03 UTC (permalink / raw) To: Jeff Liu; +Cc: xfs@oss.sgi.com Looks good in limited testing. I set some extra debug code somewhere, causing every failed mount in xfstests #078 to show up, so the original test cases were double-checked, both on the main partition (mid-upgrade) and on a backup boot partition: Before Liu Patch v2: #078, 32-bit Linux, larger partitions: PASS #078, 32-bit Linux, smaller partitions: soft oops before test could fail After Liu Patch v2: #078, 32-bit Linux, larger partitions: PASS #078, 32-bit Linux, smaller partitions: FAIL, no space left on device (CORRECT) 64-bit cases were not tested. Additionally, tests 001-005 were run (for my sake), and test 112 was run (pass). The main partition on this PC for / is XFS, and I still have a file system, so that's a good sign. A gcc compile was left running because I had to go to work, and I'll let you know if I don't have a file system when I get back home. Conclusion: Success on 32-bit, and failure is not seen for the near future. Testing in the next 24 hours may change this conclusion. Thanks! Michael On Mon, Mar 18, 2013 at 12:48 AM, Jeff Liu <jeff.liu@oracle.com> wrote: > Hello, > > Here is the v2 patch for fixing ASSERTION failed at xfs_vm_write_failed() according > to Dave's comments, so I added Dave as SOB for credit. > > Hi Michael, > Please kindly try at your at your convenience. > > Thanks, > -Jeff > This patch fix the block_offset evaluation to clear the lower 12 bits as: > block_offset = pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT > Hence, the ASSERTION should be correct because the from offset in a page > is evaluated to have the lower 12 bits only. > > Thanks Dave Chinner for pointing this out. > > Reported-by: Michael L. Semon <mlsemon35@gmail.com> > Signed-off-by: Jie Liu <jeff.liu@oracle.com> > Signed-off-by: Dave Chinner <david@fromorbit.com> > --- > fs/xfs/xfs_aops.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index 5f707e5..a418e17 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -1494,7 +1494,8 @@ xfs_vm_write_failed( > loff_t pos, > unsigned len) > { > - loff_t block_offset = pos & PAGE_MASK; > + loff_t block_offset = (pos >> PAGE_CACHE_SHIFT) << > + PAGE_CACHE_SHIFT; > loff_t block_start; > loff_t block_end; > loff_t from = pos & (PAGE_CACHE_SIZE - 1); > -- > 1.7.9.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() 2013-03-18 4:48 [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() Jeff Liu 2013-03-18 20:03 ` Michael L. Semon @ 2013-03-18 23:30 ` Dave Chinner 2013-03-19 6:08 ` Jeff Liu 1 sibling, 1 reply; 7+ messages in thread From: Dave Chinner @ 2013-03-18 23:30 UTC (permalink / raw) To: Jeff Liu; +Cc: Michael L. Semon, xfs@oss.sgi.com On Mon, Mar 18, 2013 at 12:48:16PM +0800, Jeff Liu wrote: > Hello, > > Here is the v2 patch for fixing ASSERTION failed at xfs_vm_write_failed() according > to Dave's comments, so I added Dave as SOB for credit. No, please don't add my SOB to a patch you wrote, even if I came up with the idea of how to do something. A SOB indicates that someone has verified the origin of the patch (e.g. for copyright reasons), not who contributed to finding the problem. IOWs, adding someone else's SOB to a patch you wrote is almost always the wrong thing to do. The correct way to acknowledge someone's contribution to the fix if they didn't write the patch is by a line in the commit message saying something like "Thanks to .... for helping find and fix the problem." [Edit: I just noticed you did this bit. ;) ] If .... agrees with your fix, then they can add a reviewed-by line to the patch, which has significantly different meaning to a SOB... .... > Reported-by: Michael L. Semon <mlsemon35@gmail.com> > Signed-off-by: Jie Liu <jeff.liu@oracle.com> > Signed-off-by: Dave Chinner <david@fromorbit.com> > --- > fs/xfs/xfs_aops.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index 5f707e5..a418e17 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -1494,7 +1494,8 @@ xfs_vm_write_failed( > loff_t pos, > unsigned len) > { > - loff_t block_offset = pos & PAGE_MASK; > + loff_t block_offset = (pos >> PAGE_CACHE_SHIFT) << > + PAGE_CACHE_SHIFT; > loff_t block_start; > loff_t block_end; > loff_t from = pos & (PAGE_CACHE_SIZE - 1); This needs a comment explaining why we aren't just masking the value off with PAGE_MASK. And given that it wraps, something like: - loff_t block_offset = pos & PAGE_MASK; + loff_t block_offset; ..... + /* + * comment about 32 bit systems, 64 bit variables and masks + */ + block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT; Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() 2013-03-18 23:30 ` Dave Chinner @ 2013-03-19 6:08 ` Jeff Liu 2013-03-19 19:23 ` Dave Chinner 0 siblings, 1 reply; 7+ messages in thread From: Jeff Liu @ 2013-03-19 6:08 UTC (permalink / raw) To: Dave Chinner; +Cc: Michael L. Semon, xfs@oss.sgi.com On 03/19/2013 07:30 AM, Dave Chinner wrote: > On Mon, Mar 18, 2013 at 12:48:16PM +0800, Jeff Liu wrote: >> Hello, >> >> Here is the v2 patch for fixing ASSERTION failed at xfs_vm_write_failed() according >> to Dave's comments, so I added Dave as SOB for credit. > > No, please don't add my SOB to a patch you wrote, even if I came up > with the idea of how to do something. A SOB indicates that someone > has verified the origin of the patch (e.g. for copyright reasons), > not who contributed to finding the problem. IOWs, adding someone > else's SOB to a patch you wrote is almost always the wrong thing to > do. > > The correct way to acknowledge someone's contribution to the > fix if they didn't write the patch is by a line in the commit > message saying something like "Thanks to .... for helping find and > fix the problem." [Edit: I just noticed you did this bit. ;) ] > > If .... agrees with your fix, then they can add a reviewed-by line > to the patch, which has significantly different meaning to a SOB... Ah, got it! :-P. > > .... > >> Reported-by: Michael L. Semon <mlsemon35@gmail.com> >> Signed-off-by: Jie Liu <jeff.liu@oracle.com> >> Signed-off-by: Dave Chinner <david@fromorbit.com> >> --- >> fs/xfs/xfs_aops.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c >> index 5f707e5..a418e17 100644 >> --- a/fs/xfs/xfs_aops.c >> +++ b/fs/xfs/xfs_aops.c >> @@ -1494,7 +1494,8 @@ xfs_vm_write_failed( >> loff_t pos, >> unsigned len) >> { >> - loff_t block_offset = pos & PAGE_MASK; >> + loff_t block_offset = (pos >> PAGE_CACHE_SHIFT) << >> + PAGE_CACHE_SHIFT; >> loff_t block_start; >> loff_t block_end; >> loff_t from = pos & (PAGE_CACHE_SIZE - 1); > > This needs a comment explaining why we aren't just masking the value > off with PAGE_MASK. And given that it wraps, something like: > > - loff_t block_offset = pos & PAGE_MASK; > + loff_t block_offset; > ..... > > + /* > + * comment about 32 bit systems, 64 bit variables and masks > + */ > + block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT; Indeed, the patch is improved as following: From: Jie Liu <jeff.liu@oracle.com> In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK which is 0xfffff000 as an unsigned long, that is fine on 64-bit platforms no matter the request pos is 32-bit or 64-bit. However, on 32-bit platforms, the high 32-bit in it will be masked off with (pos & PAGE_MASK) for 64-bit pos request. As a result, the evaluated block_offset is incorrect which will cause the ASSERT() failed: ASSERT(block_offset + from == pos); In this case, we can get the following kernel Panic if the CONFIG_XFS_DEBUG is enabled: [ 68.700573] XFS: Assertion failed: block_offset + from == pos, file: fs/xfs/xfs_aops.c, line: 1504 [ 68.700656] ------------[ cut here ]------------ [ 68.700692] kernel BUG at fs/xfs/xfs_message.c:100! [ 68.700742] invalid opcode: 0000 [#1] SMP ........ [ 68.701678] Pid: 4057, comm: mkfs.xfs Tainted: G O 3.9.0-rc2 #1 [ 68.701722] EIP: 0060:[<f94a7e8b>] EFLAGS: 00010282 CPU: 0 [ 68.701783] EIP is at assfail+0x2b/0x30 [xfs] [ 68.701819] EAX: 00000056 EBX: f6ef28a0 ECX: 00000007 EDX: f57d22a4 [ 68.701852] ESI: 1c2fb000 EDI: 00000000 EBP: ea6b5d30 ESP: ea6b5d1c [ 68.701895] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 [ 68.701934] CR0: 8005003b CR2: 094f3ff4 CR3: 2bcb4000 CR4: 000006f0 [ 68.701970] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 [ 68.702011] DR6: ffff0ff0 DR7: 00000400 [ 68.702046] Process mkfs.xfs (pid: 4057, ti=ea6b4000 task=ea5799e0 task.ti=ea6b4000) [ 68.702086] Stack: [ 68.702124] 00000000 f9525c48 f951fa80 f951f96b 000005e4 ea6b5d7c f9494b34 c19b0ea2 [ 68.702445] 00000066 f3d6c620 c19b0ea2 00000000 e9a91458 00001000 00000000 00000000 [ 68.702868] 00000000 c15c7e89 00000000 1c2fb000 00000000 00000000 1c2fb000 00000080 [ 68.703192] Call Trace: [ 68.703248] [<f9494b34>] xfs_vm_write_failed+0x74/0x1b0 [xfs] [ 68.703441] [<c15c7e89>] ? printk+0x4d/0x4f [ 68.703496] [<f9494d7d>] xfs_vm_write_begin+0x10d/0x170 [xfs] [ 68.703535] [<c110a34c>] generic_file_buffered_write+0xdc/0x210 [ 68.703583] [<f949b669>] xfs_file_buffered_aio_write+0xf9/0x190 [xfs] [ 68.703629] [<f949b7f3>] xfs_file_aio_write+0xf3/0x160 [xfs] [ 68.703668] [<c115e504>] do_sync_write+0x94/0xd0 [ 68.703716] [<c115ed1f>] vfs_write+0x8f/0x160 [ 68.703753] [<c115e470>] ? wait_on_retry_sync_kiocb+0x50/0x50 [ 68.703794] [<c115f017>] sys_write+0x47/0x80 [ 68.703830] [<c15d860d>] sysenter_do_call+0x12/0x28 ............. [ 68.704203] EIP: [<f94a7e8b>] assfail+0x2b/0x30 [xfs] SS:ESP 0068:ea6b5d1c [ 68.706615] ---[ end trace cdd9af4f4ecab42f ]--- [ 68.706687] Kernel panic - not syncing: Fatal exception This patch fix the block_offset evaluation to clear the lower 12 bits as: block_offset = pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT Hence, the ASSERTION should be correct because the from offset in a page is evaluated to have the lower 12 bits only. Thanks Dave Chinner for help finding and fixing this bug. Reported-by: Michael L. Semon <mlsemon35@gmail.com> Cc: Dave Chinner <david@fromorbit.com> Signed-off-by: Jie Liu <jeff.liu@oracle.com> --- fs/xfs/xfs_aops.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 5f707e5..f26a341 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -1494,13 +1494,25 @@ xfs_vm_write_failed( loff_t pos, unsigned len) { - loff_t block_offset = pos & PAGE_MASK; + loff_t block_offset; loff_t block_start; loff_t block_end; loff_t from = pos & (PAGE_CACHE_SIZE - 1); loff_t to = from + len; struct buffer_head *bh, *head; + /* + * The request pos offset might be 32 or 64 bit, this is all fine + * on 64-bit platform. However, for 64-bit pos request on 32-bit + * platform, the high 32-bit will be masked off if we evaluate the + * block_offset via (pos & PAGE_MASK) because the PAGE_MASK is + * 0xfffff000 as an unsigned long, hence the result is incorrect + * which could cause the following ASSERT failed in most cases. + * In order to avoid this, we can evaluate the block_offset with + * the lower 12-bit masked out and the ASSERT should be correct. + */ + block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT; + ASSERT(block_offset + from == pos); head = page_buffers(page); -- 1.7.9.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() 2013-03-19 6:08 ` Jeff Liu @ 2013-03-19 19:23 ` Dave Chinner 2013-03-20 2:18 ` Jeff Liu 0 siblings, 1 reply; 7+ messages in thread From: Dave Chinner @ 2013-03-19 19:23 UTC (permalink / raw) To: Jeff Liu; +Cc: Michael L. Semon, xfs@oss.sgi.com On Tue, Mar 19, 2013 at 02:08:27PM +0800, Jeff Liu wrote: > On 03/19/2013 07:30 AM, Dave Chinner wrote: > From: Jie Liu <jeff.liu@oracle.com> > > In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK > which is 0xfffff000 as an unsigned long, That's the 32 bit value. if it's a 64 bit value, it's 0xfffffffffffff000. > that is fine on 64-bit platforms no > matter the request pos is 32-bit or 64-bit. However, on 32-bit platforms, > the high 32-bit in it will be masked off with (pos & PAGE_MASK) for 64-bit pos > request. As a result, the evaluated block_offset is incorrect which will cause > the ASSERT() failed: ASSERT(block_offset + from == pos); So I'd just rearrange this slightly: > In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK > which is an unsigned long. That is fine on 64-bit platforms > regardless of whether the request pos is 32-bit or 64-bit. > However, on 32-bit platforms, the value is 0xfffff000 and so > the high 32 bits in it will be masked off with (pos & PAGE_MASK) > for a 64-bit pos As a result, the evaluated block_offset is > incorrect which will cause this failure ASSERT(block_offset + from > == pos); and potentially pass the wrong block to > xfs_vm_kill_delalloc_range(). ... > This patch fix the block_offset evaluation to clear the lower 12 bits as: > block_offset = pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT > Hence, the ASSERTION should be correct because the from offset in a page is > evaluated to have the lower 12 bits only. Saying we are clearing the lower 12 bits is not technically correct, as there are platforms with different page sizes. What we are actually calculating is the offset at the start of the page.... > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c > index 5f707e5..f26a341 100644 > --- a/fs/xfs/xfs_aops.c > +++ b/fs/xfs/xfs_aops.c > @@ -1494,13 +1494,25 @@ xfs_vm_write_failed( > loff_t pos, > unsigned len) > { > - loff_t block_offset = pos & PAGE_MASK; > + loff_t block_offset; > loff_t block_start; > loff_t block_end; > loff_t from = pos & (PAGE_CACHE_SIZE - 1); > loff_t to = from + len; > struct buffer_head *bh, *head; > > + /* > + * The request pos offset might be 32 or 64 bit, this is all fine > + * on 64-bit platform. However, for 64-bit pos request on 32-bit > + * platform, the high 32-bit will be masked off if we evaluate the > + * block_offset via (pos & PAGE_MASK) because the PAGE_MASK is > + * 0xfffff000 as an unsigned long, hence the result is incorrect > + * which could cause the following ASSERT failed in most cases. > + * In order to avoid this, we can evaluate the block_offset with > + * the lower 12-bit masked out and the ASSERT should be correct. Same here: * In order to avoid this, we can evaluate the block_offset * of the start of the page by using shifts rather than masks * the mismatch problem. > + */ > + block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT; > + > ASSERT(block_offset + from == pos); > > head = page_buffers(page); As for the code, it looks fine. Hence with the comments/commit fixups, you can add: Reviewed-by: Dave Chinner <dchinner@redhat.com> Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() 2013-03-19 19:23 ` Dave Chinner @ 2013-03-20 2:18 ` Jeff Liu 2013-04-08 21:47 ` Mark Tinguely 0 siblings, 1 reply; 7+ messages in thread From: Jeff Liu @ 2013-03-20 2:18 UTC (permalink / raw) To: Dave Chinner; +Cc: Michael L. Semon, xfs@oss.sgi.com On 03/20/2013 03:23 AM, Dave Chinner wrote: > On Tue, Mar 19, 2013 at 02:08:27PM +0800, Jeff Liu wrote: >> On 03/19/2013 07:30 AM, Dave Chinner wrote: >> From: Jie Liu <jeff.liu@oracle.com> >> >> In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK >> which is 0xfffff000 as an unsigned long, > > That's the 32 bit value. if it's a 64 bit value, it's > 0xfffffffffffff000. > >> that is fine on 64-bit platforms no >> matter the request pos is 32-bit or 64-bit. However, on 32-bit platforms, >> the high 32-bit in it will be masked off with (pos & PAGE_MASK) for 64-bit pos >> request. As a result, the evaluated block_offset is incorrect which will cause >> the ASSERT() failed: ASSERT(block_offset + from == pos); > > So I'd just rearrange this slightly: > >> In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK >> which is an unsigned long. That is fine on 64-bit platforms >> regardless of whether the request pos is 32-bit or 64-bit. >> However, on 32-bit platforms, the value is 0xfffff000 and so >> the high 32 bits in it will be masked off with (pos & PAGE_MASK) >> for a 64-bit pos As a result, the evaluated block_offset is >> incorrect which will cause this failure ASSERT(block_offset + from >> == pos); and potentially pass the wrong block to >> xfs_vm_kill_delalloc_range(). > > ... >> This patch fix the block_offset evaluation to clear the lower 12 bits as: >> block_offset = pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT >> Hence, the ASSERTION should be correct because the from offset in a page is >> evaluated to have the lower 12 bits only. > > Saying we are clearing the lower 12 bits is not technically correct, > as there are platforms with different page sizes. What we are > actually calculating is the offset at the start of the page.... > >> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c >> index 5f707e5..f26a341 100644 >> --- a/fs/xfs/xfs_aops.c >> +++ b/fs/xfs/xfs_aops.c >> @@ -1494,13 +1494,25 @@ xfs_vm_write_failed( >> loff_t pos, >> unsigned len) >> { >> - loff_t block_offset = pos & PAGE_MASK; >> + loff_t block_offset; >> loff_t block_start; >> loff_t block_end; >> loff_t from = pos & (PAGE_CACHE_SIZE - 1); >> loff_t to = from + len; >> struct buffer_head *bh, *head; >> >> + /* >> + * The request pos offset might be 32 or 64 bit, this is all fine >> + * on 64-bit platform. However, for 64-bit pos request on 32-bit >> + * platform, the high 32-bit will be masked off if we evaluate the >> + * block_offset via (pos & PAGE_MASK) because the PAGE_MASK is >> + * 0xfffff000 as an unsigned long, hence the result is incorrect >> + * which could cause the following ASSERT failed in most cases. >> + * In order to avoid this, we can evaluate the block_offset with >> + * the lower 12-bit masked out and the ASSERT should be correct. > > Same here: > > * In order to avoid this, we can evaluate the block_offset > * of the start of the page by using shifts rather than masks > * the mismatch problem. >> + */ >> + block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT; >> + >> ASSERT(block_offset + from == pos); >> >> head = page_buffers(page); > > As for the code, it looks fine. Hence with the comments/commit > fixups, you can add: > > Reviewed-by: Dave Chinner <dchinner@redhat.com> Thanks Dave for correcting me with detailed comments, the revised patch was shown as following. Regards, -Jeff In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK which is an unsigned long. That is fine on 64-bit platforms regardless of whether the request pos is 32-bit or 64-bit. However, on 32-bit platforms the value is 0xfffff000 and so the high 32 bits in it will be masked off with (pos & PAGE_MASK) for a 64-bit pos. As a result, the evaluated block_offset is incorrect which will cause this failure ASSERT(block_offset + from == pos); and potentially pass the wrong block to xfs_vm_kill_delalloc_range(). In this case, we can get the following kernel Panic if the CONFIG_XFS_DEBUG is enabled: [ 68.700573] XFS: Assertion failed: block_offset + from == pos, file: fs/xfs/xfs_aops.c, line: 1504 [ 68.700656] ------------[ cut here ]------------ [ 68.700692] kernel BUG at fs/xfs/xfs_message.c:100! [ 68.700742] invalid opcode: 0000 [#1] SMP ........ [ 68.701678] Pid: 4057, comm: mkfs.xfs Tainted: G O 3.9.0-rc2 #1 [ 68.701722] EIP: 0060:[<f94a7e8b>] EFLAGS: 00010282 CPU: 0 [ 68.701783] EIP is at assfail+0x2b/0x30 [xfs] [ 68.701819] EAX: 00000056 EBX: f6ef28a0 ECX: 00000007 EDX: f57d22a4 [ 68.701852] ESI: 1c2fb000 EDI: 00000000 EBP: ea6b5d30 ESP: ea6b5d1c [ 68.701895] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 [ 68.701934] CR0: 8005003b CR2: 094f3ff4 CR3: 2bcb4000 CR4: 000006f0 [ 68.701970] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 [ 68.702011] DR6: ffff0ff0 DR7: 00000400 [ 68.702046] Process mkfs.xfs (pid: 4057, ti=ea6b4000 task=ea5799e0 task.ti=ea6b4000) [ 68.702086] Stack: [ 68.702124] 00000000 f9525c48 f951fa80 f951f96b 000005e4 ea6b5d7c f9494b34 c19b0ea2 [ 68.702445] 00000066 f3d6c620 c19b0ea2 00000000 e9a91458 00001000 00000000 00000000 [ 68.702868] 00000000 c15c7e89 00000000 1c2fb000 00000000 00000000 1c2fb000 00000080 [ 68.703192] Call Trace: [ 68.703248] [<f9494b34>] xfs_vm_write_failed+0x74/0x1b0 [xfs] [ 68.703441] [<c15c7e89>] ? printk+0x4d/0x4f [ 68.703496] [<f9494d7d>] xfs_vm_write_begin+0x10d/0x170 [xfs] [ 68.703535] [<c110a34c>] generic_file_buffered_write+0xdc/0x210 [ 68.703583] [<f949b669>] xfs_file_buffered_aio_write+0xf9/0x190 [xfs] [ 68.703629] [<f949b7f3>] xfs_file_aio_write+0xf3/0x160 [xfs] [ 68.703668] [<c115e504>] do_sync_write+0x94/0xd0 [ 68.703716] [<c115ed1f>] vfs_write+0x8f/0x160 [ 68.703753] [<c115e470>] ? wait_on_retry_sync_kiocb+0x50/0x50 [ 68.703794] [<c115f017>] sys_write+0x47/0x80 [ 68.703830] [<c15d860d>] sysenter_do_call+0x12/0x28 ............. [ 68.704203] EIP: [<f94a7e8b>] assfail+0x2b/0x30 [xfs] SS:ESP 0068:ea6b5d1c [ 68.706615] ---[ end trace cdd9af4f4ecab42f ]--- [ 68.706687] Kernel panic - not syncing: Fatal exception In order to avoid this, we can evaluate the block_offset of the start of the page by using shifts rather than masks the mismatch problem. Thanks Dave Chinner for help finding and fixing this bug. Reported-by: Michael L. Semon <mlsemon35@gmail.com> Reviewed-by: Dave Chinner <david@fromorbit.com> Signed-off-by: Jie Liu <jeff.liu@oracle.com> --- fs/xfs/xfs_aops.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index 5f707e5..7b5d6b1 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -1494,13 +1494,26 @@ xfs_vm_write_failed( loff_t pos, unsigned len) { - loff_t block_offset = pos & PAGE_MASK; + loff_t block_offset; loff_t block_start; loff_t block_end; loff_t from = pos & (PAGE_CACHE_SIZE - 1); loff_t to = from + len; struct buffer_head *bh, *head; + /* + * The request pos offset might be 32 or 64 bit, this is all fine + * on 64-bit platform. However, for 64-bit pos request on 32-bit + * platform, the high 32-bit will be masked off if we evaluate the + * block_offset via (pos & PAGE_MASK) because the PAGE_MASK is + * 0xfffff000 as an unsigned long, hence the result is incorrect + * which could cause the following ASSERT failed in most cases. + * In order to avoid this, we can evaluate the block_offset of the + * start of the page by using shifts rather than masks the mismatch + * problem. + */ + block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT; + ASSERT(block_offset + from == pos); head = page_buffers(page); -- 1.7.9.5 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() 2013-03-20 2:18 ` Jeff Liu @ 2013-04-08 21:47 ` Mark Tinguely 0 siblings, 0 replies; 7+ messages in thread From: Mark Tinguely @ 2013-04-08 21:47 UTC (permalink / raw) To: Jeff Liu; +Cc: Michael L. Semon, xfs@oss.sgi.com >> >> Reviewed-by: Dave Chinner<dchinner@redhat.com> > > Thanks Dave for correcting me with detailed comments, the revised patch was shown as following. > > Regards, > -Jeff > > > In xfs_vm_write_failed(), we evaluate the block_offset of pos with PAGE_MASK > which is an unsigned long. That is fine on 64-bit platforms regardless of > whether the request pos is 32-bit or 64-bit. However, on 32-bit platforms > the value is 0xfffff000 and so the high 32 bits in it will be masked off with > (pos& PAGE_MASK) for a 64-bit pos. As a result, the evaluated block_offset is > incorrect which will cause this failure ASSERT(block_offset + from == pos); and > potentially pass the wrong block to xfs_vm_kill_delalloc_range(). > > In this case, we can get the following kernel Panic if the CONFIG_XFS_DEBUG is enabled: > > [ 68.700573] XFS: Assertion failed: block_offset + from == pos, file: fs/xfs/xfs_aops.c, line: 1504 > [ 68.700656] ------------[ cut here ]------------ > [ 68.700692] kernel BUG at fs/xfs/xfs_message.c:100! > [ 68.700742] invalid opcode: 0000 [#1] SMP > ........ > [ 68.701678] Pid: 4057, comm: mkfs.xfs Tainted: G O 3.9.0-rc2 #1 > [ 68.701722] EIP: 0060:[<f94a7e8b>] EFLAGS: 00010282 CPU: 0 > [ 68.701783] EIP is at assfail+0x2b/0x30 [xfs] > [ 68.701819] EAX: 00000056 EBX: f6ef28a0 ECX: 00000007 EDX: f57d22a4 > [ 68.701852] ESI: 1c2fb000 EDI: 00000000 EBP: ea6b5d30 ESP: ea6b5d1c > [ 68.701895] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 > [ 68.701934] CR0: 8005003b CR2: 094f3ff4 CR3: 2bcb4000 CR4: 000006f0 > [ 68.701970] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 > [ 68.702011] DR6: ffff0ff0 DR7: 00000400 > [ 68.702046] Process mkfs.xfs (pid: 4057, ti=ea6b4000 task=ea5799e0 task.ti=ea6b4000) > [ 68.702086] Stack: > [ 68.702124] 00000000 f9525c48 f951fa80 f951f96b 000005e4 ea6b5d7c f9494b34 c19b0ea2 > [ 68.702445] 00000066 f3d6c620 c19b0ea2 00000000 e9a91458 00001000 00000000 00000000 > [ 68.702868] 00000000 c15c7e89 00000000 1c2fb000 00000000 00000000 1c2fb000 00000080 > [ 68.703192] Call Trace: > [ 68.703248] [<f9494b34>] xfs_vm_write_failed+0x74/0x1b0 [xfs] > [ 68.703441] [<c15c7e89>] ? printk+0x4d/0x4f > [ 68.703496] [<f9494d7d>] xfs_vm_write_begin+0x10d/0x170 [xfs] > [ 68.703535] [<c110a34c>] generic_file_buffered_write+0xdc/0x210 > [ 68.703583] [<f949b669>] xfs_file_buffered_aio_write+0xf9/0x190 [xfs] > [ 68.703629] [<f949b7f3>] xfs_file_aio_write+0xf3/0x160 [xfs] > [ 68.703668] [<c115e504>] do_sync_write+0x94/0xd0 > [ 68.703716] [<c115ed1f>] vfs_write+0x8f/0x160 > [ 68.703753] [<c115e470>] ? wait_on_retry_sync_kiocb+0x50/0x50 > [ 68.703794] [<c115f017>] sys_write+0x47/0x80 > [ 68.703830] [<c15d860d>] sysenter_do_call+0x12/0x28 > ............. > [ 68.704203] EIP: [<f94a7e8b>] assfail+0x2b/0x30 [xfs] SS:ESP 0068:ea6b5d1c > [ 68.706615] ---[ end trace cdd9af4f4ecab42f ]--- > [ 68.706687] Kernel panic - not syncing: Fatal exception > > In order to avoid this, we can evaluate the block_offset of the start of the page > by using shifts rather than masks the mismatch problem. > > Thanks Dave Chinner for help finding and fixing this bug. > > Reported-by: Michael L. Semon<mlsemon35@gmail.com> > Reviewed-by: Dave Chinner<david@fromorbit.com> > Signed-off-by: Jie Liu<jeff.liu@oracle.com> > --- > fs/xfs/xfs_aops.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > Looks good. Reviewed-by: Mark Tinguely <tinguely@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-04-08 21:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-18 4:48 [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed() Jeff Liu 2013-03-18 20:03 ` Michael L. Semon 2013-03-18 23:30 ` Dave Chinner 2013-03-19 6:08 ` Jeff Liu 2013-03-19 19:23 ` Dave Chinner 2013-03-20 2:18 ` Jeff Liu 2013-04-08 21:47 ` Mark Tinguely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox