linux-kernel - Re: [PATCH 06/11] bdi: add a new writeback list for sync

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130801055931.GP7118@dastard>
Date:	Thu, 1 Aug 2013 15:59:31 +1000
From:	Dave Chinner <david@...morbit.com>
To:	Jan Kara <jack@...e.cz>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, davej@...hat.com,
	viro@...iv.linux.org.uk, glommer@...allels.com
Subject: Re: [PATCH 06/11] bdi: add a new writeback list for sync

On Wed, Jul 31, 2013 at 05:11:14PM +0200, Jan Kara wrote:
> On Wed 31-07-13 14:15:45, Dave Chinner wrote:
> >  /*
> > + * mark an inode as under writeback on the given bdi
> > + */
> > +void bdi_mark_inode_writeback(struct backing_dev_info *bdi, struct inode *inode)
> > +{
> > +	WARN_ON_ONCE(bdi != inode_to_bdi(inode));
> > +	if (list_empty(&inode->i_wb_list)) {
> > +		spin_lock(&bdi->wb.list_lock);
> > +		if (list_empty(&inode->i_wb_list))
> > +			list_add_tail(&inode->i_wb_list, &bdi->wb.b_writeback);
> > +		spin_unlock(&bdi->wb.list_lock);
> > +	}
> > +}
> > +
> > +/*
> > + * clear an inode as under writeback on the given bdi
> > + */
> > +static void bdi_clear_inode_writeback(struct backing_dev_info *bdi,
> > +				      struct inode *inode)
> > +{
> > +	WARN_ON_ONCE(bdi != inode_to_bdi(inode));
> > +	if (!list_empty(&inode->i_wb_list)) {
> > +		spin_lock(&bdi->wb.list_lock);
> > +		list_del_init(&inode->i_wb_list);
> > +		spin_unlock(&bdi->wb.list_lock);
> > +	}
> > +}
>   Umm, are these list_empty() checks without lock really safe?

I think they are.....

>   Looking into
> the code in more detail, it seems that mapping->tree_lock saves us from
> races between insert & removal but it definitely deserves a comment (or maybe
> even an assertion) that the function requires it.
> bdi_clear_inode_writeback() is safe only because it is called only when the
> inode is practically dead. Again, I think it deserves a comment...

Ok.

> > @@ -1264,9 +1330,21 @@ static void wait_sb_inodes(struct super_block *sb)
> >  
> >  		cond_resched();
> >  
> > -		spin_lock(&sb->s_inode_list_lock);
> > +                /*
> > +                 * the inode has been written back now, so check whether we
> > +                 * still have pages under IO and move it back to the primary
> > +                 * list if necessary
> > +                 */
> > +                spin_lock_irq(&mapping->tree_lock);
> > +                spin_lock(&bdi->wb.list_lock);
> > +                if (mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK)) {
> > +                        WARN_ON(list_empty(&inode->i_wb_list));
> > +                        list_move(&inode->i_wb_list, &bdi->wb.b_writeback);
> > +                } else
> > +			list_del_init(&inode->i_wb_list);
> > +                spin_unlock_irq(&mapping->tree_lock);
>   Whitespace is damaged in the above hunk...

Bizarre. I'll fix it.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/