search

OpenAoE / aoe

aoe driver for Linux with backward compatibility build system
Other
26 stars 8 forks source link

supporting discard/trim #5

Open mischief opened 8 years ago

mischief commented 8 years ago

It would be great if the driver could support passing trim commands. I believe it doesnt work right now because of a missing flag in the request_queue.

ecashin commented 8 years ago

On 03/13/2016 05:37 AM, Nick Owens wrote:

It would be great if the driver could support passing trim commands. I believe it doesnt work right now because of a missing flag in the request_queue.

Hi, Nick Owens. Did you already check whether a generic Linux block device layer allows trim commands? If not, did you imagine an out-of-band mechanism of some kind?

By out of band I mean outside the operations performed on the block device itself. Examples include /sys operations and special files like /dev/etherd/discover.

Ed

mischief commented 8 years ago

i'm pretty sure any old block device can support it.

it seems to be a matter of having the correct flags in the request_queue.

for example, nbd sets this here: https://github.com/torvalds/linux/blob/master/drivers/block/nbd.c#L750

the goal would be to mount the aoe blockdev with mount -odiscard ....

ecashin commented 8 years ago

I'm looking into it now, mostly reading Linux sources and patches so far. Have you already figured out how to get your AoE target to do the ATA that will show you're ready for the TRIM command?

mischief commented 8 years ago

yes, it was just a matter of fixing endianness of my ata identity. i missed the right one the first time through the spec :)

however, i seem to have run into a crash inside of the aoe driver when i try to use blkdiscard on the aoe device.

mischief commented 8 years ago

attached is the diff i was using to enable discard on the aoe block device if the remote ata target advertises trim in the identity.

once this is applied, simply running e.g. blkdiscard -v /dev/etherd/e1.1 will make the driver crash with a nil pointer deref. i did not have the time to dig into the nature of the issue.

0001-add-trim-support.txt

ecashin commented 8 years ago

Oh, super! Thanks for the patch. Because of the way the upstream works, can you please include a "Signed-off-by" as Documentation/SubmittingPatches describes?

Ideally, you could also fork this repo and create a pull request with a commit that is up to Andrew Morton's standards---He likes to immediately see why the users would care about the changes by reading the commit message. The short commit message specs are also described in SubmittingPatches, I think.

You might be thinking that I'm being a hypocrite, but my aoe-repo commits have terse messages only because I'm likely the one who will consolidate and summarize them before shipping them upstream. For the patches of others, it's more practical to front-load it.

Also, is your target available for others to use for testing purposes? I'm guessing it would be helpful to have it on my network as e1.1 before trying your blkdiscard command.

mischief commented 8 years ago

unfortunately, the application is not yet open source - but maybe soon.

it should be easy enough to patch vblade to make it claim TRIM support, but error when the command is sent.

mischief commented 8 years ago

attached is a patch for vblade suitable to trick the kernel's ata_id_has_trim function.

0001-fake-ata-identity.txt

ecashin commented 8 years ago

Thanks. I am going to re-do my virtual testbed a bit more and then use your patch with vblade to check it out.

ecashin commented 8 years ago

I finally got things all set up but with your patches applied to the vblade (on host two) and aoe driver (on host one) I am getting "Operation not supported" when I issue the blkdiscard command you mentioned (after building the latest utils-linux from source).

Do you think there's anything you might have forgotten to include, or maybe there's something different about my system. I'm testing with Ubuntu 14.04 on x86_64.