Re: [PATCH 2/2] xfs: add 'discard_sync' mount flag

[Eric Sandeen]

On 4/30/18 2:58 PM, Jens Axboe wrote:
> On 4/30/18 1:57 PM, Eric Sandeen wrote:
>> On 4/30/18 2:21 PM, Jens Axboe wrote:

...

>> Ok, but I'm not sure how that precludes a block device tunable?  You'd tune
>> it for your workload, right?
> 
> What kind of tunable are you thinking of? Right now we have one tunable,
> which is the max discard size. Patch #1 actually helps make this do what
> it should for sync discards, instead of just building a bio chain and
> submitting that all at once.
> 
>> Or is the concern that it could only be for the entire block device, and
>> perhaps different partitions have different workloads?
>>
>> Sorry, caveman filesystem guy doesn't completely understand block devices.
> 
> I just don't know what you are trying to tune :-)

I'm wondering if "make discards synchronous" couldn't just be a block device
tunable, rather than a filesystem mount option tunable.

-Eric

Re: [PATCH 2/2] xfs: add 'discard_sync' mount flag

[Eric Sandeen]

On 4/30/18 2:21 PM, Jens Axboe wrote:
> On 4/30/18 1:19 PM, Eric Sandeen wrote:
>>
>>
>> On 4/30/18 1:25 PM, Luis R. Rodriguez wrote:
>>> On Mon, Apr 30, 2018 at 12:07:31PM -0600, Jens Axboe wrote:
>>>> On 4/30/18 11:19 AM, Brian Foster wrote:
>>>>> On Mon, Apr 30, 2018 at 09:32:52AM -0600, Jens Axboe wrote:
>>>>>> XFS recently added support for async discards. While this can be
>>>>>> a win for some workloads and devices, there are also cases where
>>>>>> async bursty discard will severly harm the latencies of reads
>>>>>> and writes.
>>>>>>
>>>>>> Add a 'discard_sync' mount flag to revert to using sync discard,
>>>>>> issuing them one at the time and waiting for each one. This fixes
>>>>>> a big performance regression we had moving to kernels that include
>>>>>> the XFS async discard support.
>>>>>>
>>>>>> Signed-off-by: Jens Axboe <ax...@kernel.dk>
>>>>>> ---
>>>>>
>>>>> Hm, I figured the async discard stuff would have been a pretty clear win
>>>>> all around, but then again I'm not terribly familiar with what happens
>>>>> with discards beneath the fs. I do know that the previous behavior would
>>>>> cause fs level latencies due to holding up log I/O completion while
>>>>> discards completed one at a time. My understanding is that this lead to
>>>>> online discard being pretty much universally "not recommended" in favor
>>>>> of fstrim.
>>>>
>>>> It's not a secret that most devices suck at discard.
>>>
>>> How can we know if a device sucks at discard?
>>
>> I was going to ask the same thing.  ;)  "Meh, punt to the admin!"
>>
>> I'm having deja vu but can't remember why.  Seems like this has come up
>> before and we thought it should be a block device tunable, not pushed down
>> from the filesystem.  Is that possible?
> 
> The problem is that it'll depend on the workload as well. The device in
> may laptop is fine with discard for my workload, which is very light.
> But if you are running RocksDB on it, and doing heavy compactions and
> deletes, it probably would not be.

Ok, but I'm not sure how that precludes a block device tunable?  You'd tune
it for your workload, right?

Or is the concern that it could only be for the entire block device, and
perhaps different partitions have different workloads?

Sorry, caveman filesystem guy doesn't completely understand block devices.

-Eric

Re: [PATCH 2/2] xfs: add 'discard_sync' mount flag

[Eric Sandeen]

On 4/30/18 1:25 PM, Luis R. Rodriguez wrote:
> On Mon, Apr 30, 2018 at 12:07:31PM -0600, Jens Axboe wrote:
>> On 4/30/18 11:19 AM, Brian Foster wrote:
>>> On Mon, Apr 30, 2018 at 09:32:52AM -0600, Jens Axboe wrote:
 XFS recently added support for async discards. While this can be
 a win for some workloads and devices, there are also cases where
 async bursty discard will severly harm the latencies of reads
 and writes.

 Add a 'discard_sync' mount flag to revert to using sync discard,
 issuing them one at the time and waiting for each one. This fixes
 a big performance regression we had moving to kernels that include
 the XFS async discard support.

 Signed-off-by: Jens Axboe 
 ---
>>>
>>> Hm, I figured the async discard stuff would have been a pretty clear win
>>> all around, but then again I'm not terribly familiar with what happens
>>> with discards beneath the fs. I do know that the previous behavior would
>>> cause fs level latencies due to holding up log I/O completion while
>>> discards completed one at a time. My understanding is that this lead to
>>> online discard being pretty much universally "not recommended" in favor
>>> of fstrim.
>>
>> It's not a secret that most devices suck at discard.
> 
> How can we know if a device sucks at discard?

I was going to ask the same thing.  ;)  "Meh, punt to the admin!"

I'm having deja vu but can't remember why.  Seems like this has come up
before and we thought it should be a block device tunable, not pushed down
from the filesystem.  Is that possible?

-Eric

Re: [LSF/MM] Ride sharing

[Eric Sandeen]

On 4/19/18 3:34 PM, Matthew Wilcox wrote:
> I hate renting unnecessary cars, and the various transportation companies
> offer a better deal if multiple people book at once.
> 
> I'm scheduled to arrive on Sunday at 3:18pm local time if anyone wants to
> share transport.  Does anyone have a wiki we can use to coordinate this?

I get in 1:08pm local time on Sunday.  I'd be game to coordinate if possible,
but it may be too late for that.

FWIW I spoke with http://canyontransport.com and although the website says
minimum 2 adults, they will actually take just one, so you don't /have/ to
find another just to book with them.  The price is per-person regardless
of how many are in the van.

-Eric

Re: [PATCH] mkfs: Remove messages printed when blocksize < physical sectorsize

[Eric Sandeen]

On 9/5/17 5:10 PM, Dave Chinner wrote:
> On Tue, Sep 05, 2017 at 09:17:42AM -0500, Eric Sandeen wrote:
>>
>>
>> On 9/5/17 1:44 AM, Dave Chinner wrote:
>>> On Mon, Sep 04, 2017 at 11:31:39PM -0700, Christoph Hellwig wrote:
>>>> On Tue, Sep 05, 2017 at 11:14:42AM +0530, Chandan Rajendra wrote:
>>>>> Linux kernel commit 6c6b6f28b3335fd85ec833ee0005d9c9dca6c003 (loop: set
>>>>> physical block size to PAGE_SIZE) now sets PAGE_SIZE as the default
>>>>> physical sector size of loop devices. On ppc64, this causes loop devices
>>>>> to have 64k as the physical sector size.
>>>>
>>>> Eek.  We'll need to revert the loop change ASAP!
>>>
>>> And, FWIW, making the warning go away if probably a bad idea,
>>> because XFS only supports devices with sector sizes up
>>> to 32k:
>>
>> Well, TBH removing this warning was my suggestion, because it's
>> automatically fixing values that weren't specified by the user in
>> the first place.  First preference is physical sector size, then
>> fallback to logical but it doesn't need to be noisy about it.
>>
>>> #define XFS_MIN_SECTORSIZE_LOG  9   /* i.e. 512 bytes */
>>> #define XFS_MAX_SECTORSIZE_LOG  15  /* i.e. 32768 bytes */
>>>
>>> And so it should be warning about devices trying to tell it to use
>>> something larger
>>
>> As long as the logical sector size is small enough, it seems like a
>> silent adjustment is probably ok,  no?
> 
> Think 512e drives. Doing 512 byte sector IO is possible, but slow.
> Someone might actually want to avoid that by having the filesystem
> use 4k sector sizes. However, if for some reason, mkfs selects 512
> byte sectors (the logical size) rather than 4k sector size, then
> shouldn't we be telling the user we're doing something that has a
> "for-the-life-of-the-filesystem" performance impact?

Well, sure, but it'll only select 512 if it /has/ to, i.e. if the
block size is < 4k.

So for the simple case of a 512e drive, our default block size is
4k, physical size is 4k, and everything is happy and fine.

If the user /specifies/ a 1k block size on such a device, how much
of a nanny do we really want to be about telling them this is suboptimal?

There are a lot of suboptimal things you can specify on the
mkfs commandline, but we don't generally choose to warn about them...

-Eric

> Cheers,
> 
> Dave.
> 

Re: [PATCH] mkfs: Remove messages printed when blocksize < physical sectorsize

[Eric Sandeen]

On 9/5/17 1:44 AM, Dave Chinner wrote:
> On Mon, Sep 04, 2017 at 11:31:39PM -0700, Christoph Hellwig wrote:
>> On Tue, Sep 05, 2017 at 11:14:42AM +0530, Chandan Rajendra wrote:
>>> Linux kernel commit 6c6b6f28b3335fd85ec833ee0005d9c9dca6c003 (loop: set
>>> physical block size to PAGE_SIZE) now sets PAGE_SIZE as the default
>>> physical sector size of loop devices. On ppc64, this causes loop devices
>>> to have 64k as the physical sector size.
>>
>> Eek.  We'll need to revert the loop change ASAP!
> 
> And, FWIW, making the warning go away if probably a bad idea,
> because XFS only supports devices with sector sizes up
> to 32k:

Well, TBH removing this warning was my suggestion, because it's
automatically fixing values that weren't specified by the user in
the first place.  First preference is physical sector size, then
fallback to logical but it doesn't need to be noisy about it.

> #define XFS_MIN_SECTORSIZE_LOG  9   /* i.e. 512 bytes */
> #define XFS_MAX_SECTORSIZE_LOG  15  /* i.e. 32768 bytes */
> 
> And so it should be warning about devices trying to tell it to use
> something larger

As long as the logical sector size is small enough, it seems like a
silent adjustment is probably ok,  no?

-Eric

> Cheers,
> 
> Dave.
> 

Re: [PATCH 0/6] RFC add blkdev tests v2

[Eric Sandeen]

On 4/6/17 10:20 AM, Johannes Thumshirn wrote:
> On Thu, Apr 06, 2017 at 08:51:36AM -0600, Jens Axboe wrote:
>> On 04/06/2017 08:47 AM, Christoph Hellwig wrote:
>>> On Thu, Apr 06, 2017 at 08:33:55AM -0600, Jens Axboe wrote:
 That is exactly what my recommendation was at lsfmm as well - fork
 xfstest, prune bits we don't need, and off we go. I'll get around to
 it soonish.
>>>
>>> So you volunteer to do it?  I was -><- this close to offering it myself,
>>> and than thought of all the other backlog I have on my plate :)
>>
>> Yeah, I'll get something cobbled together next week.
> 
> Yay!
> 
> But please keep the dependencies small. It'll suck when we'd have to pull in a
> gazillion megabytes for perl/pyhthon/whatever.

Full-blown xfstests doesn't have /that/ much in the way of deps,
on debian it's ostensibly just:

sudo apt-get install xfslibs-dev uuid-dev libtool-bin \
e2fsprogs automake gcc libuuid1 quota attr libattr1-dev make \
libacl1-dev libaio-dev xfsprogs libgdbm-dev gawk fio dbench \
uuid-runtime

and lots of that might not be needed for non-fs tests.

Otherwise well, you do need bash ... ;)

-Eric