linux-ext4 - Re: do not disable ext4 discards on first discard failure? [was: Re: dm snapshot: ignore discards issued to the snapshot-origin target]

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: <20110504180343.GB558@redhat.com>
Date:	Wed, 4 May 2011 14:03:44 -0400
From:	Mike Snitzer <snitzer@...hat.com>
To:	"Martin K. Petersen" <martin.petersen@...cle.com>
Cc:	sandeen@...hat.com, Christoph Hellwig <hch@...radead.org>,
	device-mapper development <dm-devel@...hat.com>,
	DarkNovaNick@...il.com, linux-lvm@...hat.com,
	Lukas Czerner <lczerner@...hat.com>,
	linux-ext4@...r.kernel.org, Alasdair G Kergon <agk@...hat.com>
Subject: Re: do not disable ext4 discards on first discard failure? [was: Re:
 dm snapshot: ignore discards issued to the snapshot-origin target]

On Wed, May 04 2011 at 12:50pm -0400,
Martin K. Petersen <martin.petersen@...cle.com> wrote:

> >>>>> "Mike" == Mike Snitzer <snitzer@...hat.com> writes:
> 
> Mike> lim->discard_zeroes_data = -1; was suspect to me too.
> Mike> But why default to 1 here?
> 
> Because otherwise DM would default to having dzd to "unsupported",
> meaning the feature would never be turned on regardless of the bottom
> device capabilities.
> 
> The old approach used the -1 value to indicate "has not been set". That
> was only really intended as a value for the stacking drivers, not for
> the LLDs. It was a bit of a hack and I'd rather deal with dzd the same
> way as we do with clustering.
> 
> 
> >> @@ -166,6 +166,7 @@ void blk_queue_make_request(struct request_queue
> >> *q, make_request_fn *mfn)
> >> 
> >> blk_set_default_limits(&q->limits);
> >> blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
> >> + q->limits.discard_zeroes_data = 0;
> >> 
> >> /*
> >> * by default assume old behaviour and bounce for any highmem page
> 
> Mike> Only to then reset to 0 here?  Shouldn't we default to 0 and only
> Mike> set to 1 where applicable (e.g. sd_config_discard)?
> 
> My first approach was to set it in dm-table.c before stacking. But I
> thought it was icky to have the stacking driver ask for defaults and
> then have to tweak them for things to work correctly.
> 
> The other option is to have blk_set_default_stacking_limits(). Or we
> could add a flag to blk_set_default_limits to indicate whether this is a
> LLD or a stacking driver.
> 
> We already special-case BLK_SAFE_MAX_SECTORS when setting the request
> function. And that's the only non-stacking user of the default limits
> call. So that's why I disabled dzd there. Since this is a stable bugfix
> I also wanted to keep it small and simple. But I'm totally open to
> suggestions.

Your current approach sounds good.  Might be good to briefly speak to
the duality of the stacking vs non-stacking approach in the associated
patch header.

Thanks for clarifying.

Mike
--
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