| Message ID | alpine.LFD.2.00.1304031503470.10110@localhost |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wed, Apr 03, 2013 at 04:41:01PM +0200, Lukáš Czerner wrote: > Hi Ted, > > there is a problem with your patch: > > 32d90a241a44d22ebc5289d2a2561691fc2d1351 > > because there is one more case where we might call ext4_truncate() > without i_mutex locked - from ext4_symlink(). Because we might be > calling __page_symlink() and it will call ext4_write_begin(). In > possible error case (ENOSPC for example) we might want to truncate > everything which might have been instantiated past i_size, however > at that point we're not holding i_mutex because there is no point in > doing so - the inode can not be possibly held by anyone else. > > My proposal is to only check whether the mutex is locked if the > inode is _not_ new or is _not_ being freed. > > There is a quick&dirty patch and it seems to be working well so > far. Let me know if you prefer standalone patch, or if you'll > include it into your commit. I really like your approach. We've been trying a number of other approaches, including http://patchwork.ozlabs.org/patch/232914/ and http://patchwork.ozlabs.org/patch/231679/ but I think your suggestion is the best. Thanks!! - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 03, 2013 at 04:41:01PM +0200, Lukáš Czerner wrote: > > There is a quick&dirty patch and it seems to be working well so > far. Let me know if you prefer standalone patch, or if you'll > include it into your commit. While rebasing the ext4 dev tree on top of the 3.9 emergency big-endian fix (which has been accpeted into Linus's tree), I've folded your propose change into my commit. Thanks!! - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -255,21 +255,8 @@ void ext4_evict_inode(struct inode *inode) "couldn't mark inode dirty (err %d)", err); goto stop_handle; } - if (inode->i_blocks) { - /* - * Since we are evicting the inode, it shouldn't be - * locked. We've added a warning which triggers if - * the mutex is not locked, so take the lock even - * though it's not strictly necessary. However, - * taking the lock using a simple mutex_lock() will - * trigger a (false positive) lockdep warning, so take - * it using a trylock. - */ - int locked = mutex_trylock(&inode->i_mutex); + if (inode->i_blocks) ext4_truncate(inode); - if (likely(locked)) - mutex_unlock(&inode->i_mutex); - } /* * ext4_ext_truncate() doesn't reserve any slop when it @@ -3690,7 +3677,13 @@ void ext4_truncate(struct inode *inode) handle_t *handle; struct address_space *mapping = inode->i_mapping; - WARN_ON(!mutex_is_locked(&inode->i_mutex)); + /* + * There is a possibility that we're either freeing the inode + * or it completely new indode. In those cases we might not + * have i_mutex locked because it's not necessary. + */ + if (!(inode->i_state & (I_NEW|I_FREEING))) + WARN_ON(!mutex_is_locked(&inode->i_mutex)); trace_ext4_truncate_enter(inode); if (!ext4_can_truncate(inode))