| Date | Sat, 16 Oct 2010 18:54:21 +1100 | From | Nick Piggin <> | Subject | Re: [PATCH 04/17] fs: icache lock i_state |
| |
On Fri, Oct 01, 2010 at 01:54:33AM -0400, Christoph Hellwig wrote: > > + spin_lock(&inode->i_lock); > > + if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) > > + || inode->i_mapping->nrpages == 0) { > > > This is some pretty strange formatting. > > if ((inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) || > inode->i_mapping->nrpages == 0) { > > would be more standard. > > > list_for_each_entry(inode, &sb->s_inodes, i_sb_list) { > > struct address_space *mapping; > > > > - if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) > > - continue; > > mapping = inode->i_mapping; > > if (mapping->nrpages == 0) > > continue; > > + spin_lock(&inode->i_lock); > > + if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW)) { > > + spin_unlock(&inode->i_lock); > > + continue; > > + } > > Can we access the mapping safely when the inode isn't actually fully > setup? Even if we can I'd rather not introduce this change hidden > inside an unrelated patch.
Good point, fixed.
|