[PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants

[PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants

[Jan Kara]

evict_inodes() and invalidate_inodes() use list_for_each_entry_safe()
to iterate sb->s_inodes list. However, since we use i_lru list entry for
our local temporary list of inodes to destroy, the inode is guaranteed
to stay in sb->s_inodes list while we hold sb->s_inode_list_lock. So
there is no real need for safe iteration variant and we can use
list_for_each_entry() just fine.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/inode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 88110fd0b282..bd5a47ff2f03 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -601,12 +601,12 @@ static void dispose_list(struct list_head *head)
  */
 void evict_inodes(struct super_block *sb)
 {
-	struct inode *inode, *next;
+	struct inode *inode;
 	LIST_HEAD(dispose);
 
 again:
 	spin_lock(&sb->s_inode_list_lock);
-	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
 		if (atomic_read(&inode->i_count))
 			continue;
 
@@ -651,11 +651,11 @@ void evict_inodes(struct super_block *sb)
 int invalidate_inodes(struct super_block *sb, bool kill_dirty)
 {
 	int busy = 0;
-	struct inode *inode, *next;
+	struct inode *inode;
 	LIST_HEAD(dispose);
 
 	spin_lock(&sb->s_inode_list_lock);
-	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
 		spin_lock(&inode->i_lock);
 		if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
 			spin_unlock(&inode->i_lock);
-- 
2.10.2

Re: [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2419 bytes --]

On Jan 10, 2017, at 5:27 AM, Jan Kara <jack@suse.cz> wrote:
> 
> evict_inodes() and invalidate_inodes() use list_for_each_entry_safe()
> to iterate sb->s_inodes list. However, since we use i_lru list entry for
> our local temporary list of inodes to destroy, the inode is guaranteed
> to stay in sb->s_inodes list while we hold sb->s_inode_list_lock. So
> there is no real need for safe iteration variant and we can use
> list_for_each_entry() just fine.

This is a pretty "subtle" change, IMHO, with little benefit.  IMHO, using
the "_safe" variant makes it more clear to the reader that the inode is
being deleted from the list.  At a minimum, I'd think there should be a
comment at list_for_each_entry() to the effect that the inode is not going
to be deleted, so list_for_each_entry_safe() is not needed.  Otherwise, if
the inode lifetime changes in some manner in the future this may introduce
hard-to-find corruption of freed memory.

Cheers, Andreas

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/inode.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 88110fd0b282..bd5a47ff2f03 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -601,12 +601,12 @@ static void dispose_list(struct list_head *head)
>  */
> void evict_inodes(struct super_block *sb)
> {
> -	struct inode *inode, *next;
> +	struct inode *inode;
> 	LIST_HEAD(dispose);
> 
> again:
> 	spin_lock(&sb->s_inode_list_lock);
> -	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> 		if (atomic_read(&inode->i_count))
> 			continue;
> 
> @@ -651,11 +651,11 @@ void evict_inodes(struct super_block *sb)
> int invalidate_inodes(struct super_block *sb, bool kill_dirty)
> {
> 	int busy = 0;
> -	struct inode *inode, *next;
> +	struct inode *inode;
> 	LIST_HEAD(dispose);
> 
> 	spin_lock(&sb->s_inode_list_lock);
> -	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> 		spin_lock(&inode->i_lock);
> 		if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
> 			spin_unlock(&inode->i_lock);
> --
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants

[Jan Kara]

On Tue 10-01-17 12:10:27, Andreas Dilger wrote:
> On Jan 10, 2017, at 5:27 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > evict_inodes() and invalidate_inodes() use list_for_each_entry_safe()
> > to iterate sb->s_inodes list. However, since we use i_lru list entry for
> > our local temporary list of inodes to destroy, the inode is guaranteed
> > to stay in sb->s_inodes list while we hold sb->s_inode_list_lock. So
> > there is no real need for safe iteration variant and we can use
> > list_for_each_entry() just fine.
> 
> This is a pretty "subtle" change, IMHO, with little benefit.  IMHO, using
> the "_safe" variant makes it more clear to the reader that the inode is
> being deleted from the list.  At a minimum, I'd think there should be a
> comment at list_for_each_entry() to the effect that the inode is not going
> to be deleted, so list_for_each_entry_safe() is not needed.  Otherwise, if
> the inode lifetime changes in some manner in the future this may introduce
> hard-to-find corruption of freed memory.

Well, but the inode is not deleted from the list we iterate (and that is
pretty obvious from the loop body) so using a _safe variant looks just
confusing to me... I can add a comment if people think that will help
readability of the code.

								Honza

> 
> Cheers, Andreas
> 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/inode.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 88110fd0b282..bd5a47ff2f03 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -601,12 +601,12 @@ static void dispose_list(struct list_head *head)
> >  */
> > void evict_inodes(struct super_block *sb)
> > {
> > -	struct inode *inode, *next;
> > +	struct inode *inode;
> > 	LIST_HEAD(dispose);
> > 
> > again:
> > 	spin_lock(&sb->s_inode_list_lock);
> > -	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> > +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> > 		if (atomic_read(&inode->i_count))
> > 			continue;
> > 
> > @@ -651,11 +651,11 @@ void evict_inodes(struct super_block *sb)
> > int invalidate_inodes(struct super_block *sb, bool kill_dirty)
> > {
> > 	int busy = 0;
> > -	struct inode *inode, *next;
> > +	struct inode *inode;
> > 	LIST_HEAD(dispose);
> > 
> > 	spin_lock(&sb->s_inode_list_lock);
> > -	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> > +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> > 		spin_lock(&inode->i_lock);
> > 		if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
> > 			spin_unlock(&inode->i_lock);
> > --
> > 2.10.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR