Skip to content

Conversation

corentinbaron
Copy link
Contributor

new version, using ioctl(_fd, BLKBSZGET) and fixed predefined alignment for PPC64.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails compilation on non-Linux platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I'm a Linux dev, I'd be glad to know the equivalent on other systems. I'll propose it again with these and their calls between #ifdefs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a set of #includes in the same file for non-Windows code around lines 160-165. You can move these #includes there and wrap linux/fs.h with linux.

@benety
Copy link
Contributor

benety commented Sep 5, 2014

@benety
Copy link
Contributor

benety commented Sep 5, 2014

Hi Corentin,

Thank you for submitting this pull request - this looks more promising than #735.

There are a couple of issues (see inlined comments) to resolve before I hand this over to the other reviewers.

I was able to rebase your patch on a6b9d6d. For future reference, if the patch applies to both master and 2.6, please submit against master and we'll back port to the 2.6 branch when the pull request is merged.

Thanks
Ben

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make g_minOSPageSizeBytes the default for _blkSize and move the ioctl call into the the O_DIRECT #ifdef...#endif block around lines 186-195

@benety
Copy link
Contributor

benety commented Sep 5, 2014

Hi @corentinbaron,

I've added some additional feedback (see inlined comments). Hopefully this will get back to working across all platforms.

Ben

@benety
Copy link
Contributor

benety commented Sep 10, 2014

Replaced by #769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants