* [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants
@ 2017-01-10 12:27 Jan Kara
2017-01-10 19:10 ` Andreas Dilger
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2017-01-10 12:27 UTC (permalink / raw)
To: Al Viro; +Cc: linux-fsdevel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants
2017-01-10 12:27 [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants Jan Kara
@ 2017-01-10 19:10 ` Andreas Dilger
2017-01-11 8:30 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Andreas Dilger @ 2017-01-10 19:10 UTC (permalink / raw)
To: Jan Kara; +Cc: Al Viro, linux-fsdevel
[-- 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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants
2017-01-10 19:10 ` Andreas Dilger
@ 2017-01-11 8:30 ` Jan Kara
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2017-01-11 8:30 UTC (permalink / raw)
To: Andreas Dilger; +Cc: Jan Kara, Al Viro, linux-fsdevel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-01-11 8:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-10 12:27 [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants Jan Kara
2017-01-10 19:10 ` Andreas Dilger
2017-01-11 8:30 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).