linux-ext4 - Re: ext4_orphan_del() sleeps in non-journal mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 15 Sep 2012 21:54:30 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Anatol Pomozov <anatol.pomozov@...il.com>
Cc:	Dmitry Monakhov <dmonakhov@...nvz.org>, linux-ext4@...r.kernel.org
Subject: Re: ext4_orphan_del() sleeps in non-journal mode

On Sat, Sep 15, 2012 at 03:28:19PM -0700, Anatol Pomozov wrote:
> 
> But the other more general issue "ext4_orphan_del sleeps in no-journal
> mode" still applies. As Dmitry mentioned in this commit 3d287de3b828
> such sleep might degrade performance. In no-journal mode we do not
> need to manipulate with i_orphan list and no reason to take the mutex....
> 
> In this example we take handle and important thing to note here is
> that IS_ERR(handle) can be true only in journal mode when starting
> journal failed. In non-journal mode ext4_journal_start() *always*
> returns a fake handle that is non-error (see ext4_get_nojournal). So
> the example above never sleeps in ext4_orphan_del().

Instead of trying to "fix" this at all of the call sites for
ext4_orphan_del(), the better way to fix this something like this:

-	/* ext4_handle_valid() assumes a valid handle_t pointer */
-	if (handle && !ext4_handle_valid(handle))
-	   return 0;
+	sbi = EXT4_SB(inode->i_sb);
+	if (!sbi->journal)
+		return 0;

I don't consider it a high priority fir for upstream, since all of the
places where ext4_orphan_del(NULL, ..) is called are in error paths,
where performance is not critical.  However, it should fix the problem
you're working on.

As far as the the local Google patch to 2.6.34, it might be worth
looking to see if it's still worth upstreaming it in the light of
commit 4bd809dbb: "ext4: don't take the i_mutex lock when doing DIO
overwrites".  It's been a while since I've looked that the DIO fast
path changes, but if it's worth geting upstream, we should do that ---
but we _will_ need to make it workable for file systems with journals.
(Which is someting I'd really like to fix for Google kernels, since
leaving things broken for file systems with journal is a bad ju-ju ---
at the very least we should disable the fast path codepath if one of
our customers try to mount file system with a journal.)

Cheers,

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