Messages in this thread |  | | | Date | Fri, 8 Oct 2010 03:42:43 -0400 | | From | Christoph Hellwig <> | | Subject | Re: [PATCH 13/18] fs: split locking of inode writeback and LRU lists |
| |
On Fri, Oct 08, 2010 at 04:21:27PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > Now that the inode LRU and IO lists are split apart, we can separate > the locking for them. The IO lists are only ever accessed in the > context of writeback, so a per-BDI lock for those lists separates > them out nicely.
I think this description needs some updates. It seems like it's from Nick's original patch that splits the lock, but at this point we still have inode_lock anyway.
> > -static inline struct backing_dev_info *inode_to_bdi(struct inode *inode) > -{ > - struct super_block *sb = inode->i_sb; > - > - if (strcmp(sb->s_type->name, "bdev") == 0) > - return inode->i_mapping->a_bdi; > - > - return sb->s_bdi; > -}
Please don't extent the scope of this one. Just add a new inode_wb_del or similar helper to remove and inode from the writeback list.
> struct inode *inode = list_entry(wb->b_io.prev, > @@ -475,7 +475,6 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb, > redirty_tail(inode); > continue; > } > - > /* > * The inode belongs to a different superblock. > * Bounce back to the caller to unpin this and
spurious whitespace change.
> @@ -484,7 +483,7 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb, > return 0; > } > > - if (inode->i_state & (I_NEW | I_WILL_FREE)) { > + if (inode->i_state & (I_NEW | I_WILL_FREE | I_FREEING)) { > requeue_io(inode); > continue; > }
What does this have to do with the rest of the patch?
> @@ -495,8 +494,11 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb, > if (inode_dirtied_after(inode, wbc->wb_start)) > return 1; > > - BUG_ON(inode->i_state & I_FREEING); > + spin_lock(&inode->i_lock); > iref_locked(inode); > + spin_unlock(&inode->i_lock);
Shouldn't this become a plain iref now?
> +/* > + * check against I_FREEING as inode writeback completion could race with > + * setting the I_FREEING and removing the inode from the LRU. > + */ > +void inode_lru_list_add(struct inode *inode) > +{ > + spin_lock(&inode_lru_lock); > + if (list_empty(&inode->i_lru) && !(inode->i_state & I_FREEING)) { > + list_add(&inode->i_lru, &inode_lru); > + percpu_counter_inc(&nr_inodes_unused); > + } > + spin_unlock(&inode_lru_lock); > +}
Ah, here you introduce the lru list helpers I suggested earlier. Moving them earlier in the series probably is a good idea to avoid exporting nr_inodes_unused, even if the locking for the helpers will change in this patch.
|  |