Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mount r/w GCE PD disks with -o discard #28448

Merged
merged 1 commit into from
Jul 7, 2016
Merged

Mount r/w GCE PD disks with -o discard #28448

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/volume/gce_pd/attacher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ func (attacher *gcePersistentDiskAttacher) MountDevice(spec *volume.Spec, device
options := []string{}
if readOnly {
options = append(options, "ro")
} else {
// as per https://cloud.google.com/compute/docs/disks/add-persistent-disk#formatting
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to test for ext4 also. I don't think ext2 (nor implictly ext3 supports this mount option)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. This just got complicated.

On Wed, Jul 6, 2016 at 11:51 PM, Michael Rubin notifications@github.com
wrote:

In pkg/volume/gce_pd/attacher.go
#28448 (comment)
:

@@ -165,6 +165,9 @@ func (attacher *gcePersistentDiskAttacher) MountDevice(spec *volume.Spec, device
options := []string{}
if readOnly {
options = append(options, "ro")

  • } else {
  •   // as per https://cloud.google.com/compute/docs/disks/add-persistent-disk#formatting

We need to test for ext4 also. I don't think ext2 (nor implictly ext3
supports this mount option)?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28448/files/8efefab9a3fdc4906ca53d4a23d22c3846d0610b#r69858276,
or mute the thread
https://github.com/notifications/unsubscribe/AFVgVG1VbgifAwcL_sXxrRhsOd1YPG9Dks5qTKIEgaJpZM4JECrD
.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thockin @saad-ali @ciwang Can't you just use "file -s"?

Copy link
Contributor

Choose a reason for hiding this comment

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

@thockin @saad-ali @ciwang Can't you just use "file -s"?

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd have to explain more what you're thinking.

On Thu, Jul 7, 2016 at 12:14 AM, Michael Rubin notifications@github.com
wrote:

In pkg/volume/gce_pd/attacher.go
#28448 (comment)
:

@@ -165,6 +165,9 @@ func (attacher *gcePersistentDiskAttacher) MountDevice(spec *volume.Spec, device
options := []string{}
if readOnly {
options = append(options, "ro")

  • } else {
  •   // as per https://cloud.google.com/compute/docs/disks/add-persistent-disk#formatting

@thockin https://github.com/thockin @saad-ali
https://github.com/saad-ali @ciwang https://github.com/ciwang Can't
you just use "file -s"?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28448/files/8efefab9a3fdc4906ca53d4a23d22c3846d0610b#r69860194,
or mute the thread
https://github.com/notifications/unsubscribe/AFVgVMuhjJb2CDIZeLUmPmD6RanfX6Dvks5qTKdggaJpZM4JECrD
.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thockin @ciwang @saad-ali
Can we invoke file -s on the block device so that we can determine the filesystem type in a portable manner?
Check out: http://www.thegeekstuff.com/2011/04/identify-file-system-type/
This might be a good PR for ciwang's stack of intern items. Or does "file" not exist in our image?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh! Yeah, we could, I'm just wary of too many "is ext4" conditionals
spread throughout the code makeing assumptions about what flags other parts
did or did not use.

On Thu, Jul 7, 2016 at 12:39 AM, Michael Rubin notifications@github.com
wrote:

In pkg/volume/gce_pd/attacher.go
#28448 (comment)
:

@@ -165,6 +165,9 @@ func (attacher *gcePersistentDiskAttacher) MountDevice(spec *volume.Spec, device
options := []string{}
if readOnly {
options = append(options, "ro")

  • } else {
  •   // as per https://cloud.google.com/compute/docs/disks/add-persistent-disk#formatting

@thockin https://github.com/thockin @ciwang https://github.com/ciwang
@saad-ali https://github.com/saad-ali
Can we invoke file -s http://linux.die.net/man/1/file on the block
device so that we can determine the filesystem type in a portable manner?
Check out: http://www.thegeekstuff.com/2011/04/identify-file-system-type/
This might be a good PR for ciwang's stack of intern items. Or does "file"
not exist in our image?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28448/files/8efefab9a3fdc4906ca53d4a23d22c3846d0610b#r69863021,
or mute the thread
https://github.com/notifications/unsubscribe/AFVgVGT2iHT8jQF0IdjDo2mBquiaRFSLks5qTK0rgaJpZM4JECrD
.

options = append(options, "discard")
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the same documentation don't we also want to do this for mkfs?
Do we need to add it also in [mount_linx.go]?(https://github.com/kubernetes/kubernetes/blob/master/pkg/util/mount/mount_linux.go#L282)?

Copy link
Member Author

Choose a reason for hiding this comment

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

pkg/util/mount is used by more than GCE, and I don't know if that option should be used on real disks...

Copy link
Contributor

Choose a reason for hiding this comment

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

So you just made me look at the kernel source code again.

We should not run mkfs if we don't know that the device supports TRIM.

You can see in fs/ext4/mballoc.c that if DISCARD is set but not supported in the device the kernel will emit a warning to dmesg, even just for mount. I think if we cannot test for SSD or discard supported device we should not set this.
There is no way to set mkfs flags per device?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this mean we should not mount with trim if we did not mkfs with trim?
as things stand today, there is no way to pass custom flags to mkfs. But
it looks like GCE suggests setting it regardless of SSD-ness (since PD is
...unique)

On Wed, Jul 6, 2016 at 11:45 PM, Michael Rubin notifications@github.com
wrote:

In pkg/volume/gce_pd/attacher.go
#28448 (comment)
:

@@ -165,6 +165,9 @@ func (attacher *gcePersistentDiskAttacher) MountDevice(spec *volume.Spec, device
options := []string{}
if readOnly {
options = append(options, "ro")

  • } else {
  •   // as per https://cloud.google.com/compute/docs/disks/add-persistent-disk#formatting

  •   options = append(options, "discard")

So you just made me look at the kernel source code again.

We should not run mkfs if we don't know that the device supports TRIM.

You can see in fs/ext4/mballoc.c that if DISCARD is set but not supported
in the device the kernel will emit a warning to dmesg, even just for mount.
I think if we cannot test for SSD or discard supported device we should not
set this.
There is no way to set mkfs flags per device?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28448/files/8efefab9a3fdc4906ca53d4a23d22c3846d0610b#r69857853,
or mute the thread
https://github.com/notifications/unsubscribe/AFVgVICvE6nVIqxXld383IVXtADLn7maks5qTKCYgaJpZM4JECrD
.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I looked at it even deeper. enabling this in mkfs only speeds up mkfs, it has no fs impact once mounted. We get benefits of "discard" in the mount option. So we don't need to do both.

}
if notMnt {
diskMounter := &mount.SafeFormatAndMount{Interface: mounter, Runner: exec.New()}
Expand Down