linux-ext4 - Re: [PATCH,RFC 1/7] ext4: fold __mpage_da_writepage() into write_cache_pages

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]

Date:	Sun, 13 Feb 2011 07:48:19 -0500
From:	Josef Bacik <josef@...hat.com>
To:	"Ted Ts'o" <tytso@....edu>
Cc:	Josef Bacik <josef@...hat.com>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH,RFC 1/7] ext4: fold __mpage_da_writepage() into
	write_cache_pages_da()

On Sun, Feb 13, 2011 at 12:42:35AM -0500, Ted Ts'o wrote:
> On Sat, Feb 12, 2011 at 08:25:29PM -0500, Josef Bacik wrote:
> > > +out:
> > > +	pagevec_release(&pvec);
> > > +	cond_resched();
> > > +	return ret;
> > >  }
> > 
> > Do we really need the cond_resched() here?  Seems like it will just add
> > unwanted/uneeded latencies.
> 
> The cond_resched is from the original write_cache_pages(), and if you
> follow the code movement, it goes all the way back to fs/mpage.c's
> __mpage_writepages() from 2.6.11 (the beginning of time as far as the
> Linux 2.6's git repository is concerned).
> 
> The basic idea is that given that writeback threads are basically
> running in a tight loop trying to push out dirty pages, you need to
> eventually give other processes a chance to run --- especially on a UP
> system!  I do wonder whether we are checking way too much, though.
> The cond_resched() I'd be tempted to take out is not the one at the
> end of the function, but the one at the end of the while loop.
> 
> That would allow us to complete the the writeback for a particular
> inode before letting another process run, which would trade off
> efficiency for a bit more scheduling unfairness.  But given that a
> particular writeback call is capped at writing out a relatively small
> mount of data anyway, that would seem to be OK.
> 
> But even XFS has a cond_resched in xfs_cluster_write() (in
> fs/xfs/linux-2.6/xfs_aops.c) so I'd want to do a lot of thinking,
> testing, and benchmarking before removing that call to cond_resched().
>

Ah I didn't look at anybody else.  My thinking was we only really need it in one
place, and we have it in the while() loop.  But you are right, it probably makes
more sense to drop the one in the while loop and then have it before we go back
to the main writeback code.  Thanks,

Josef 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html