linux-kernel - Re: [PATCH 1/4] bdi: Track users that require stable page writes

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: <20121121215207.GB32202@blackbox.djwong.org>
Date:	Wed, 21 Nov 2012 13:52:07 -0800
From:	"Darrick J. Wong" <darrick.wong@...cle.com>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	axboe@...nel.dk, lucho@...kov.net, jack@...e.cz, ericvh@...il.com,
	tytso@....edu, rminnich@...dia.gov, viro@...iv.linux.org.uk,
	martin.petersen@...cle.com, neilb@...e.de, david@...morbit.com,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	adilger.kernel@...ger.ca, bharrosh@...asas.com, jlayton@...ba.org,
	v9fs-developer@...ts.sourceforge.net, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/4] bdi: Track users that require stable page writes

Ok, I'll update the description a bit.

On Wed, Nov 21, 2012 at 05:56:24AM -0500, Christoph Hellwig wrote:
> > +static inline void bdi_require_stable_pages(struct backing_dev_info *bdi)
> > +{
> > +	bdi->capabilities |= BDI_CAP_STABLE_WRITES;
> > +}
> > +
> > +static inline void bdi_unrequire_stable_pages(struct backing_dev_info *bdi)
> > +{
> > +	bdi->capabilities &= ~BDI_CAP_STABLE_WRITES;
> > +}
> 
> Any reason to provide these wrappers while other BDI_CAP_ values don't
> have it/
> 
> Also what protects bdi->capabilities against concurrent updates now that
> it gets modified at runtime?

Nothing seems to update ->capabilities at run time.

That said, if you're really worried about concurrent updates, I can always put
a spinlock around all the updates.

(Or revert to the atomic_t counter, but that seemed unpopular...)

I think I can drop the wrappers.

> > +static inline void queue_require_stable_pages(struct request_queue *q)
> > +{
> > +	bdi_require_stable_pages(&q->backing_dev_info);
> > +}
> > +
> > +static inline void queue_unrequire_stable_pages(struct request_queue *q)
> > +{
> > +	bdi_unrequire_stable_pages(&q->backing_dev_info);
> > +}
> > +
> > +static inline int queue_requires_stable_pages(struct request_queue *q)
> > +{
> > +	return bdi_cap_stable_pages_required(&q->backing_dev_info);
> > +}
> 
> Independent of the above I see no point in these wrappers that just
> provide a single dereference.
> 
> > +static ssize_t stable_pages_required_store(struct device *dev,
> > +					   struct device_attribute *attr,
> > +					   const char *buf, size_t count)
> 
> Can you add a rationale on why we'd want to allow users to change the
> value?  I can't really think of any.

I dislike the idea that if a program is dirtying pages that are being written
out, then I don't really know whether the disk will write the before or after
version.  If the power goes out before the inevitable second write, how do you
know which version you get?  Sure would be nice if I could force on stable
writes if I'm feeling paranoid.

--D
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/