* [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze()
@ 2013-05-24 8:57 Vahram Martirosyan
2013-05-24 8:57 ` [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function Vahram Martirosyan
2013-05-24 17:03 ` [Jfs-discussion] [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Dave Kleikamp
0 siblings, 2 replies; 6+ messages in thread
From: Vahram Martirosyan @ 2013-05-24 8:57 UTC (permalink / raw)
To: Dave Kleikamp
Cc: Vahram Martirosyan, jfs-discussion, linux-kernel, spruce-project,
Gu Zheng
The mentioned functions do not pay attention to the error codes returned
by the functions updateSuper(), lmLogInit() and lmLogShutdown(). It brings to
system crash later when writing to log.
The patch adds corresponding code to check and return the error codes
and to print correct error messages in case of errors.
Besides that the lmLogShutdown() function must not be called when 'nointegrity' mount option is provided.
It leads to kernel OOPS.
Found by Linux File System Verification project (linuxtesting.org).
Signed-off-by: Vahram Martirosyan <vahram.martirosyan@linuxtesting.org>
Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
fs/jfs/super.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 2003e83..a3d424d 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -611,11 +611,20 @@ static int jfs_freeze(struct super_block *sb)
{
struct jfs_sb_info *sbi = JFS_SBI(sb);
struct jfs_log *log = sbi->log;
+ int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
txQuiesce(sb);
- lmLogShutdown(log);
- updateSuper(sb, FM_CLEAN);
+ rc = lmLogShutdown(log);
+ if (rc != 0) {
+ jfs_err("lmLogShutdown failed with return code %d", rc);
+ return rc;
+ }
+ rc = updateSuper(sb, FM_CLEAN);
+ if (rc != 0) {
+ jfs_err("updateSuper failed with return code %d", rc);
+ return rc;
+ }
}
return 0;
}
@@ -627,11 +636,17 @@ static int jfs_unfreeze(struct super_block *sb)
int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
- updateSuper(sb, FM_MOUNT);
- if ((rc = lmLogInit(log)))
- jfs_err("jfs_unlock failed with return code %d", rc);
- else
- txResume(sb);
+ rc = updateSuper(sb, FM_MOUNT);
+ if (rc != 0) {
+ jfs_err("updateSuper failed with return code %d", rc);
+ return rc;
+ }
+ rc = lmLogInit(log);
+ if (rc != 0) {
+ jfs_err("lmLogInit failed with return code %d", rc);
+ return rc;
+ }
+ txResume(sb);
}
return 0;
}
--
1.8.2.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function
2013-05-24 8:57 [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Vahram Martirosyan
@ 2013-05-24 8:57 ` Vahram Martirosyan
2013-05-24 9:25 ` Gu Zheng
2013-05-24 17:03 ` [Jfs-discussion] [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Dave Kleikamp
1 sibling, 1 reply; 6+ messages in thread
From: Vahram Martirosyan @ 2013-05-24 8:57 UTC (permalink / raw)
To: Dave Kleikamp
Cc: Vahram Martirosyan, jfs-discussion, linux-kernel, spruce-project,
Gu Zheng
In function jfs_freeze() the log is shut down through lmLogShutdown() call.
When the "nointegrity" mount option is enabled, the log is actually not
initialized. As a result the freeze operation in that case brings to a
kernel OOPS.
The solution is to check if the "nointegrity" option is enabled and if it is not
then shut the log down.
May be this is not the best solution, but at least it fixes the OOPS.
Found by Linux File System Verification project (linuxtesting.org)
Signed-off-by: Vahram Martirosyan <vahram.martirosyan@linuxtesting.org>
---
fs/jfs/super.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index a3d424d..9788970 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -615,10 +615,12 @@ static int jfs_freeze(struct super_block *sb)
if (!(sb->s_flags & MS_RDONLY)) {
txQuiesce(sb);
- rc = lmLogShutdown(log);
- if (rc != 0) {
- jfs_err("lmLogShutdown failed with return code %d", rc);
- return rc;
+ if (!log->no_integrity) {
+ rc = lmLogShutdown(log);
+ if (rc != 0) {
+ jfs_err("lmLogShutdown failed with return code %d", rc);
+ return rc;
+ }
}
rc = updateSuper(sb, FM_CLEAN);
if (rc != 0) {
--
1.8.2.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function
2013-05-24 8:57 ` [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function Vahram Martirosyan
@ 2013-05-24 9:25 ` Gu Zheng
2013-05-24 20:32 ` Dave Kleikamp
0 siblings, 1 reply; 6+ messages in thread
From: Gu Zheng @ 2013-05-24 9:25 UTC (permalink / raw)
To: Vahram Martirosyan
Cc: Dave Kleikamp, Vahram Martirosyan, jfs-discussion, linux-kernel,
spruce-project
[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]
Hi Vahram,
I saw the same issue in the bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=53331,
and I sent out a patch this issue, but I've get any feedback.
In fact, I think it's the right way to fix this issue,
can you help to test it?
Thanks,
Gu
On 05/24/2013 04:57 PM, Vahram Martirosyan wrote:
> In function jfs_freeze() the log is shut down through lmLogShutdown() call.
> When the "nointegrity" mount option is enabled, the log is actually not
> initialized. As a result the freeze operation in that case brings to a
> kernel OOPS.
>
> The solution is to check if the "nointegrity" option is enabled and if it is not
> then shut the log down.
>
> May be this is not the best solution, but at least it fixes the OOPS.
>
> Found by Linux File System Verification project (linuxtesting.org)
>
> Signed-off-by: Vahram Martirosyan <vahram.martirosyan@linuxtesting.org>
> ---
> fs/jfs/super.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index a3d424d..9788970 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -615,10 +615,12 @@ static int jfs_freeze(struct super_block *sb)
>
> if (!(sb->s_flags & MS_RDONLY)) {
> txQuiesce(sb);
> - rc = lmLogShutdown(log);
> - if (rc != 0) {
> - jfs_err("lmLogShutdown failed with return code %d", rc);
> - return rc;
> + if (!log->no_integrity) {
> + rc = lmLogShutdown(log);
> + if (rc != 0) {
> + jfs_err("lmLogShutdown failed with return code %d", rc);
> + return rc;
> + }
> }
> rc = updateSuper(sb, FM_CLEAN);
> if (rc != 0) {
[-- Attachment #2: 0001-fs-jfs-Add-check-if-journaling-to-disk-has-been-disa.patch --]
[-- Type: text/plain, Size: 936 bytes --]
>From 2f1fb676cfc98602706eb8aaa1638272b8da8dfb Mon Sep 17 00:00:00 2001
From: Gu Zheng <guz.fnst@cn.fujitsu.com>
Date: Thu, 23 May 2013 16:14:19 +0800
Subject: [PATCH] fs/jfs: Add check if journaling to disk has been disabled in
lbmRead()
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
fs/jfs/jfs_logmgr.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c
index c57499d..360d27c 100644
--- a/fs/jfs/jfs_logmgr.c
+++ b/fs/jfs/jfs_logmgr.c
@@ -2009,7 +2009,13 @@ static int lbmRead(struct jfs_log * log, int pn, struct lbuf ** bpp)
bio->bi_end_io = lbmIODone;
bio->bi_private = bp;
- submit_bio(READ_SYNC, bio);
+ /*check if journaling to disk has been disabled*/
+ if (log->no_integrity) {
+ bio->bi_size = 0;
+ lbmIODone(bio, 0);
+ } else {
+ submit_bio(READ_SYNC, bio);
+ }
wait_event(bp->l_ioevent, (bp->l_flag != lbmREAD));
--
1.7.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Jfs-discussion] [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze()
2013-05-24 8:57 [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Vahram Martirosyan
2013-05-24 8:57 ` [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function Vahram Martirosyan
@ 2013-05-24 17:03 ` Dave Kleikamp
2013-05-24 21:01 ` [PATCH] " Dave Kleikamp
1 sibling, 1 reply; 6+ messages in thread
From: Dave Kleikamp @ 2013-05-24 17:03 UTC (permalink / raw)
To: Vahram Martirosyan
Cc: Dave Kleikamp, Gu Zheng, Vahram Martirosyan, jfs-discussion,
spruce-project, linux-kernel
On 05/24/2013 03:57 AM, Vahram Martirosyan wrote:
> The mentioned functions do not pay attention to the error codes returned
> by the functions updateSuper(), lmLogInit() and lmLogShutdown(). It brings to
> system crash later when writing to log.
>
> The patch adds corresponding code to check and return the error codes
> and to print correct error messages in case of errors.
>
> Besides that the lmLogShutdown() function must not be called when 'nointegrity' mount option is provided.
> It leads to kernel OOPS.
>
> Found by Linux File System Verification project (linuxtesting.org).
>
> Signed-off-by: Vahram Martirosyan <vahram.martirosyan@linuxtesting.org>
>
> Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
> fs/jfs/super.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
> index 2003e83..a3d424d 100644
> --- a/fs/jfs/super.c
> +++ b/fs/jfs/super.c
> @@ -611,11 +611,20 @@ static int jfs_freeze(struct super_block *sb)
> {
> struct jfs_sb_info *sbi = JFS_SBI(sb);
> struct jfs_log *log = sbi->log;
> + int rc = 0;
>
> if (!(sb->s_flags & MS_RDONLY)) {
> txQuiesce(sb);
> - lmLogShutdown(log);
> - updateSuper(sb, FM_CLEAN);
> + rc = lmLogShutdown(log);
> + if (rc != 0) {
> + jfs_err("lmLogShutdown failed with return code %d", rc);
> + return rc;
Hmm. I'm not sure about the nicest way to recover here. txQuiesce() has
been called. This will leave the filesystem effectively frozen, but not
marked as frozen. Maybe better to call jfs_error() and maybe txResume()
in an attempt to unblock any blocked threads.
> + }
> + rc = updateSuper(sb, FM_CLEAN);
> + if (rc != 0) {
> + jfs_err("updateSuper failed with return code %d", rc);
> + return rc;
same here, except we know that lmLogShutdown() has already succeeded.
Maybe we're better off ignoring this return code, since the freezing
succeeded. Failing to mark the filesystem clean is hardly damaging. It
just means that fsck will work harder next time, which is probably a
good idea. This is an unlikely error if everything else has succeeded.
> + }
> }
> return 0;
> }
> @@ -627,11 +636,17 @@ static int jfs_unfreeze(struct super_block *sb)
> int rc = 0;
>
> if (!(sb->s_flags & MS_RDONLY)) {
> - updateSuper(sb, FM_MOUNT);
> - if ((rc = lmLogInit(log)))
> - jfs_err("jfs_unlock failed with return code %d", rc);
> - else
> - txResume(sb);
> + rc = updateSuper(sb, FM_MOUNT);
> + if (rc != 0) {
> + jfs_err("updateSuper failed with return code %d", rc);
> + return rc;
I think I like calling jfs_error() here and letting the system panic,
the filesystem to be marked as read-only (default), or whatever action
is desired.
> + }
> + rc = lmLogInit(log);
> + if (rc != 0) {
> + jfs_err("lmLogInit failed with return code %d", rc);
> + return rc;
same here
> + }
> + txResume(sb);
> }
> return 0;
> }
I'm going to tweak your patch a bit and see what kind of improvement I
can make.
Thanks,
Shaggy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function
2013-05-24 9:25 ` Gu Zheng
@ 2013-05-24 20:32 ` Dave Kleikamp
0 siblings, 0 replies; 6+ messages in thread
From: Dave Kleikamp @ 2013-05-24 20:32 UTC (permalink / raw)
To: Gu Zheng
Cc: Vahram Martirosyan, Dave Kleikamp, Vahram Martirosyan,
jfs-discussion, linux-kernel, spruce-project
On 05/24/2013 04:25 AM, Gu Zheng wrote:
> Hi Vahram,
> I saw the same issue in the bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=53331,
> and I sent out a patch this issue, but I've get any feedback.
Sorry I missed that bug. I just realized that bugzilla.kernel.org has
been sending my email to my old IBM address. I've got some catching up
to do.
> In fact, I think it's the right way to fix this issue,
> can you help to test it?
I might go with your patch for now, as it is an improvement, but I don't
really like the way that lmLogShutdown is skipped altogether during a
"nointegrity" unmount. If we skip it during a "nointegrity" freeze, we
should also skip lmLogInit on "nointegrity" unfreeze. I'd like to keep
the nointegrity logic in jfs_logmgr.c, but it would be easiest to fix in
the freeze and unfreeze functions. A code cleanup may be better than the
easy fix. I'll have to work on that.
It also looks like freeze/unfreeze doesn't take into account that two or
more mounted file systems can share the same journal. I don't know how
often, if ever, this is done in practice, but it looks like it would be
a problem. Two "nointegrity" mounts may have problem too. That should be
easy enough to verify.
Thanks,
Shaggy
>
> Thanks,
> Gu
>
>
> On 05/24/2013 04:57 PM, Vahram Martirosyan wrote:
>
>> In function jfs_freeze() the log is shut down through lmLogShutdown() call.
>> When the "nointegrity" mount option is enabled, the log is actually not
>> initialized. As a result the freeze operation in that case brings to a
>> kernel OOPS.
>>
>> The solution is to check if the "nointegrity" option is enabled and if it is not
>> then shut the log down.
>>
>> May be this is not the best solution, but at least it fixes the OOPS.
>>
>> Found by Linux File System Verification project (linuxtesting.org)
>>
>> Signed-off-by: Vahram Martirosyan <vahram.martirosyan@linuxtesting.org>
>> ---
>> fs/jfs/super.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/jfs/super.c b/fs/jfs/super.c
>> index a3d424d..9788970 100644
>> --- a/fs/jfs/super.c
>> +++ b/fs/jfs/super.c
>> @@ -615,10 +615,12 @@ static int jfs_freeze(struct super_block *sb)
>>
>> if (!(sb->s_flags & MS_RDONLY)) {
>> txQuiesce(sb);
>> - rc = lmLogShutdown(log);
>> - if (rc != 0) {
>> - jfs_err("lmLogShutdown failed with return code %d", rc);
>> - return rc;
>> + if (!log->no_integrity) {
>> + rc = lmLogShutdown(log);
>> + if (rc != 0) {
>> + jfs_err("lmLogShutdown failed with return code %d", rc);
>> + return rc;
>> + }
>> }
>> rc = updateSuper(sb, FM_CLEAN);
>> if (rc != 0) {
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] jfs: Several bugs in jfs_freeze() and jfs_unfreeze()
2013-05-24 17:03 ` [Jfs-discussion] [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Dave Kleikamp
@ 2013-05-24 21:01 ` Dave Kleikamp
0 siblings, 0 replies; 6+ messages in thread
From: Dave Kleikamp @ 2013-05-24 21:01 UTC (permalink / raw)
To: Vahram Martirosyan
Cc: jfs-discussion, linux-kernel, Vahram Martirosyan, Gu Zheng,
spruce-project
Vahram,
This is your first patch modified to be a little more robust. If there is
no objection, I'll push it to linux-next along with Gu's nointegrity
patch.
From: Vahram Martirosyan <vmartirosyan@gmail.com>
Date: Fri, 24 May 2013 13:57:12 +0500
The mentioned functions do not pay attention to the error codes returned
by the functions updateSuper(), lmLogInit() and lmLogShutdown(). It brings
to system crash later when writing to log.
The patch adds corresponding code to check and return the error codes
and to print correct error messages in case of errors.
Found by Linux File System Verification project (linuxtesting.org).
Signed-off-by: Vahram Martirosyan <vahram.martirosyan@linuxtesting.org>
Reviewed-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
---
fs/jfs/super.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/fs/jfs/super.c b/fs/jfs/super.c
index 2003e83..788e0a9 100644
--- a/fs/jfs/super.c
+++ b/fs/jfs/super.c
@@ -611,11 +611,28 @@ static int jfs_freeze(struct super_block *sb)
{
struct jfs_sb_info *sbi = JFS_SBI(sb);
struct jfs_log *log = sbi->log;
+ int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
txQuiesce(sb);
- lmLogShutdown(log);
- updateSuper(sb, FM_CLEAN);
+ rc = lmLogShutdown(log);
+ if (rc) {
+ jfs_error(sb, "jfs_freeze: lmLogShutdown failed");
+
+ /* let operations fail rather than hang */
+ txResume(sb);
+
+ return rc;
+ }
+ rc = updateSuper(sb, FM_CLEAN);
+ if (rc) {
+ jfs_err("jfs_freeze: updateSuper failed\n");
+ /*
+ * Don't fail here. Everything succeeded except
+ * marking the superblock clean, so there's really
+ * no harm in leaving it frozen for now.
+ */
+ }
}
return 0;
}
@@ -627,13 +644,18 @@ static int jfs_unfreeze(struct super_block *sb)
int rc = 0;
if (!(sb->s_flags & MS_RDONLY)) {
- updateSuper(sb, FM_MOUNT);
- if ((rc = lmLogInit(log)))
- jfs_err("jfs_unlock failed with return code %d", rc);
- else
- txResume(sb);
+ rc = updateSuper(sb, FM_MOUNT);
+ if (rc) {
+ jfs_error(sb, "jfs_unfreeze: updateSuper failed");
+ goto out;
+ }
+ rc = lmLogInit(log);
+ if (rc)
+ jfs_error(sb, "jfs_unfreeze: lmLogInit failed");
+out:
+ txResume(sb);
}
- return 0;
+ return rc;
}
static struct dentry *jfs_do_mount(struct file_system_type *fs_type,
--
1.8.2.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-24 21:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-24 8:57 [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Vahram Martirosyan
2013-05-24 8:57 ` [PATCH 2/2] jfs: Log shutdown error in jfs_freeze() function Vahram Martirosyan
2013-05-24 9:25 ` Gu Zheng
2013-05-24 20:32 ` Dave Kleikamp
2013-05-24 17:03 ` [Jfs-discussion] [PATCH 1/2] jfs: Several bugs in jfs_freeze() and jfs_unfreeze() Dave Kleikamp
2013-05-24 21:01 ` [PATCH] " Dave Kleikamp
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox