* [PATCH v2 03/18] fs: Treat foreign mounts as nosuid
From: Seth Forshee @ 2015-12-07 21:21 UTC (permalink / raw)
To: Eric W. Biederman, Alexander Viro, Serge Hallyn, James Morris,
Serge E. Hallyn, Paul Moore, Stephen Smalley, Eric Paris
Cc: Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux,
Seth Forshee, Andy Lutomirski
In-Reply-To: <1449523289-144238-1-git-send-email-seth.forshee@canonical.com>
From: Andy Lutomirski <luto@amacapital.net>
If a process gets access to a mount from a different user
namespace, that process should not be able to take advantage of
setuid files or selinux entrypoints from that filesystem. Prevent
this by treating mounts from other mount namespaces and those not
owned by current_user_ns() or an ancestor as nosuid.
This will make it safer to allow more complex filesystems to be
mounted in non-root user namespaces.
This does not remove the need for MNT_LOCK_NOSUID. The setuid,
setgid, and file capability bits can no longer be abused if code in
a user namespace were to clear nosuid on an untrusted filesystem,
but this patch, by itself, is insufficient to protect the system
from abuse of files that, when execed, would increase MAC privilege.
As a more concrete explanation, any task that can manipulate a
vfsmount associated with a given user namespace already has
capabilities in that namespace and all of its descendents. If they
can cause a malicious setuid, setgid, or file-caps executable to
appear in that mount, then that executable will only allow them to
elevate privileges in exactly the set of namespaces in which they
are already privileges.
On the other hand, if they can cause a malicious executable to
appear with a dangerous MAC label, running it could change the
caller's security context in a way that should not have been
possible, even inside the namespace in which the task is confined.
As a hardening measure, this would have made CVE-2014-5207 much
more difficult to exploit.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: James Morris <james.l.morris@oracle.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
---
fs/exec.c | 2 +-
fs/namespace.c | 13 +++++++++++++
include/linux/mount.h | 1 +
security/commoncap.c | 2 +-
security/selinux/hooks.c | 2 +-
5 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index b06623a9347f..ea7311d72cc3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1295,7 +1295,7 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
bprm->cred->euid = current_euid();
bprm->cred->egid = current_egid();
- if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+ if (!mnt_may_suid(bprm->file->f_path.mnt))
return;
if (task_no_new_privs(current))
diff --git a/fs/namespace.c b/fs/namespace.c
index da70f7c4ece1..2101ce7b96ab 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3276,6 +3276,19 @@ found:
return visible;
}
+bool mnt_may_suid(struct vfsmount *mnt)
+{
+ /*
+ * Foreign mounts (accessed via fchdir or through /proc
+ * symlinks) are always treated as if they are nosuid. This
+ * prevents namespaces from trusting potentially unsafe
+ * suid/sgid bits, file caps, or security labels that originate
+ * in other namespaces.
+ */
+ return !(mnt->mnt_flags & MNT_NOSUID) && check_mnt(real_mount(mnt)) &&
+ in_userns(current_user_ns(), mnt->mnt_sb->s_user_ns);
+}
+
static struct ns_common *mntns_get(struct task_struct *task)
{
struct ns_common *ns = NULL;
diff --git a/include/linux/mount.h b/include/linux/mount.h
index f822c3c11377..54a594d49733 100644
--- a/include/linux/mount.h
+++ b/include/linux/mount.h
@@ -81,6 +81,7 @@ extern void mntput(struct vfsmount *mnt);
extern struct vfsmount *mntget(struct vfsmount *mnt);
extern struct vfsmount *mnt_clone_internal(struct path *path);
extern int __mnt_is_readonly(struct vfsmount *mnt);
+extern bool mnt_may_suid(struct vfsmount *mnt);
struct path;
extern struct vfsmount *clone_private_mount(struct path *path);
diff --git a/security/commoncap.c b/security/commoncap.c
index 400aa224b491..6243aef5860e 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -448,7 +448,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
if (!file_caps_enabled)
return 0;
- if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+ if (!mnt_may_suid(bprm->file->f_path.mnt))
return 0;
if (!in_userns(current_user_ns(), bprm->file->f_path.mnt->mnt_sb->s_user_ns))
return 0;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d0cfaa9f19d0..a5b93df6553f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2171,7 +2171,7 @@ static int check_nnp_nosuid(const struct linux_binprm *bprm,
const struct task_security_struct *new_tsec)
{
int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS);
- int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID);
+ int nosuid = !mnt_may_suid(bprm->file->f_path.mnt);
int rc;
if (!nnp && !nosuid)
--
1.9.1
^ permalink raw reply related
* [PATCH v2 02/18] block_dev: Check permissions towards block device inode when mounting
From: Seth Forshee @ 2015-12-07 21:21 UTC (permalink / raw)
To: Eric W. Biederman, Alexander Viro
Cc: Serge Hallyn, Seth Forshee, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
Miklos Szeredi, linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-bcache-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
fuse-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Austin S Hemmelgarn,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
selinux-+05T5uksL2qpZYMLLGbcSA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1449523289-144238-1-git-send-email-seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Unprivileged users should not be able to mount block devices when
they lack sufficient privileges towards the block device inode.
Update blkdev_get_by_path() to validate that the user has the
required access to the inode at the specified path. The check
will be skipped for CAP_SYS_ADMIN, so privileged mounts will
continue working as before.
Signed-off-by: Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
fs/block_dev.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 3ebbde85d898..4fdb6ab59816 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1424,9 +1424,14 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
void *holder)
{
struct block_device *bdev;
+ int perm = 0;
int err;
- bdev = lookup_bdev(path, 0);
+ if (mode & FMODE_READ)
+ perm |= MAY_READ;
+ if (mode & FMODE_WRITE)
+ perm |= MAY_WRITE;
+ bdev = lookup_bdev(path, perm);
if (IS_ERR(bdev))
return bdev;
--
1.9.1
------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
^ permalink raw reply related
* [PATCH v2 01/18] block_dev: Support checking inode permissions in lookup_bdev()
From: Seth Forshee @ 2015-12-07 21:21 UTC (permalink / raw)
To: Eric W. Biederman, Kent Overstreet, Alasdair Kergon, Mike Snitzer,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA, Neil Brown, David Woodhouse,
Brian Norris, Alexander Viro, Jan Kara, Jeff Layton,
J. Bruce Fields
Cc: Serge Hallyn, Seth Forshee, Miklos Szeredi,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-bcache-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
fuse-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Austin S Hemmelgarn,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
selinux-+05T5uksL2qpZYMLLGbcSA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1449523289-144238-1-git-send-email-seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
When looking up a block device by path no permission check is
done to verify that the user has access to the block device inode
at the specified path. In some cases it may be necessary to
check permissions towards the inode, such as allowing
unprivileged users to mount block devices in user namespaces.
Add an argument to lookup_bdev() to optionally perform this
permission check. A value of 0 skips the permission check and
behaves the same as before. A non-zero value specifies the mask
of access rights required towards the inode at the specified
path. The check is always skipped if the user has CAP_SYS_ADMIN.
All callers of lookup_bdev() currently pass a mask of 0, so this
patch results in no functional change. Subsequent patches will
add permission checks where appropriate.
Signed-off-by: Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
drivers/md/bcache/super.c | 2 +-
drivers/md/dm-table.c | 2 +-
drivers/mtd/mtdsuper.c | 2 +-
fs/block_dev.c | 13 ++++++++++---
fs/quota/quota.c | 2 +-
include/linux/fs.h | 2 +-
6 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 679a093a3bf6..e8287b0d1dac 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1926,7 +1926,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr,
sb);
if (IS_ERR(bdev)) {
if (bdev == ERR_PTR(-EBUSY)) {
- bdev = lookup_bdev(strim(path));
+ bdev = lookup_bdev(strim(path), 0);
mutex_lock(&bch_register_lock);
if (!IS_ERR(bdev) && bch_is_open(bdev))
err = "device already registered";
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 061152a43730..81c60b2495ed 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -380,7 +380,7 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
BUG_ON(!t);
/* convert the path to a device */
- bdev = lookup_bdev(path);
+ bdev = lookup_bdev(path, 0);
if (IS_ERR(bdev)) {
dev = name_to_dev_t(path);
if (!dev)
diff --git a/drivers/mtd/mtdsuper.c b/drivers/mtd/mtdsuper.c
index 20c02a3b7417..b5b60e1af31c 100644
--- a/drivers/mtd/mtdsuper.c
+++ b/drivers/mtd/mtdsuper.c
@@ -176,7 +176,7 @@ struct dentry *mount_mtd(struct file_system_type *fs_type, int flags,
/* try the old way - the hack where we allowed users to mount
* /dev/mtdblock$(n) but didn't actually _use_ the blockdev
*/
- bdev = lookup_bdev(dev_name);
+ bdev = lookup_bdev(dev_name, 0);
if (IS_ERR(bdev)) {
ret = PTR_ERR(bdev);
pr_debug("MTDSB: lookup_bdev() returned %d\n", ret);
diff --git a/fs/block_dev.c b/fs/block_dev.c
index f90d91efa1b4..3ebbde85d898 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1426,7 +1426,7 @@ struct block_device *blkdev_get_by_path(const char *path, fmode_t mode,
struct block_device *bdev;
int err;
- bdev = lookup_bdev(path);
+ bdev = lookup_bdev(path, 0);
if (IS_ERR(bdev))
return bdev;
@@ -1736,12 +1736,14 @@ EXPORT_SYMBOL(ioctl_by_bdev);
/**
* lookup_bdev - lookup a struct block_device by name
* @pathname: special file representing the block device
+ * @mask: rights to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC)
*
* Get a reference to the blockdevice at @pathname in the current
* namespace if possible and return it. Return ERR_PTR(error)
- * otherwise.
+ * otherwise. If @mask is non-zero, check for access rights to the
+ * inode at @pathname.
*/
-struct block_device *lookup_bdev(const char *pathname)
+struct block_device *lookup_bdev(const char *pathname, int mask)
{
struct block_device *bdev;
struct inode *inode;
@@ -1756,6 +1758,11 @@ struct block_device *lookup_bdev(const char *pathname)
return ERR_PTR(error);
inode = d_backing_inode(path.dentry);
+ if (mask != 0 && !capable(CAP_SYS_ADMIN)) {
+ error = __inode_permission(inode, mask);
+ if (error)
+ goto fail;
+ }
error = -ENOTBLK;
if (!S_ISBLK(inode->i_mode))
goto fail;
diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 3746367098fd..a40eaecbd5cc 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -733,7 +733,7 @@ static struct super_block *quotactl_block(const char __user *special, int cmd)
if (IS_ERR(tmp))
return ERR_CAST(tmp);
- bdev = lookup_bdev(tmp->name);
+ bdev = lookup_bdev(tmp->name, 0);
putname(tmp);
if (IS_ERR(bdev))
return ERR_CAST(bdev);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8a17c5649ef2..879ec382fd88 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2373,7 +2373,7 @@ static inline void unregister_chrdev(unsigned int major, const char *name)
#define BLKDEV_MAJOR_HASH_SIZE 255
extern const char *__bdevname(dev_t, char *buffer);
extern const char *bdevname(struct block_device *bdev, char *buffer);
-extern struct block_device *lookup_bdev(const char *);
+extern struct block_device *lookup_bdev(const char *, int mask);
extern void blkdev_show(struct seq_file *,off_t);
#else
--
1.9.1
------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
^ permalink raw reply related
* [PATCH v2 00/19] Support fuse mounts in user namespaces
From: Seth Forshee @ 2015-12-07 21:21 UTC (permalink / raw)
To: Eric W. Biederman, linux-bcache, dm-devel, linux-raid, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
Cc: Alexander Viro, Serge Hallyn, Richard Weinberger,
Austin S Hemmelgarn, Miklos Szeredi, linux-kernel, Seth Forshee
These patches implement support for mounting filesystems in user
namespaces using fuse. They are based on the patches in the for-testing
branch of
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git,
but I've rebased them onto 4.4-rc3. I've pushed all of this to:
git://git.kernel.org/pub/scm/linux/kernel/git/sforshee/linux.git fuse-userns
The patches are organized into three high-level groups.
Patches 1-6 are related to security, adding restrictions for
unprivileged mounts and updating the LSMs as needed. Patches 1-2
(checking inode permissions for block device mounts) may not be strictly
necessary for fuseblk mounts since fuse doesn't do any IO on the block
device in the kernel, but it still seems like a good idea to fail the
mount if the user doesn't have the required permissions for the inode
(though this is a bit misleading with fuse since the mounts are done via
a suid-root helper).
Patches 7-14 update most of the vfs to translate ids correctly and deal
with inodes which may have invalid user/group ids. I've omitted patches
for anything not used by fuse - quota, fs freezing, some helper
functions, etc. - but if these are wanted for the sake of completeness I
can include them.
Patches 15-18 update fuse to deal with mounts from non-init pid and user
namespaces and enable mounting from user namespaces.
Changes since v1:
- Drop patch for FIBMAP.
- Use current_in_userns in fuse_allow_current_process.
- Remove checks for uid/gid validity in fuse. Intead, ids from the
backing store which do not map into s_user_ns will result in invalid
ids in the vfs inode. Checks in the vfs will prevent unmappable ids
from being passed in from above.
- Update a couple of commit messages to provide more detail about
changes.
Thanks,
Seth
Andy Lutomirski (1):
fs: Treat foreign mounts as nosuid
Seth Forshee (17):
block_dev: Support checking inode permissions in lookup_bdev()
block_dev: Check permissions towards block device inode when mounting
selinux: Add support for unprivileged mounts from user namespaces
userns: Replace in_userns with current_in_userns
Smack: Handle labels consistently in untrusted mounts
fs: Check for invalid i_uid in may_follow_link()
cred: Reject inodes with invalid ids in set_create_file_as()
fs: Refuse uid/gid changes which don't map into s_user_ns
fs: Update posix_acl support to handle user namespace mounts
fs: Ensure the mounter of a filesystem is privileged towards its
inodes
fs: Don't remove suid for CAP_FSETID in s_user_ns
fs: Allow superblock owner to access do_remount_sb()
capabilities: Allow privileged user in s_user_ns to set security.*
xattrs
fuse: Add support for pid namespaces
fuse: Support fuse filesystems outside of init_user_ns
fuse: Restrict allow_other to the superblock's namespace or a
descendant
fuse: Allow user namespace mounts
drivers/md/bcache/super.c | 2 +-
drivers/md/dm-table.c | 2 +-
drivers/mtd/mtdsuper.c | 2 +-
fs/attr.c | 11 +++++++
fs/block_dev.c | 18 +++++++++--
fs/exec.c | 2 +-
fs/fuse/cuse.c | 3 +-
fs/fuse/dev.c | 26 ++++++++++++----
fs/fuse/dir.c | 16 +++++-----
fs/fuse/file.c | 22 +++++++++++---
fs/fuse/fuse_i.h | 10 +++++-
fs/fuse/inode.c | 42 +++++++++++++++++---------
fs/inode.c | 6 +++-
fs/namei.c | 2 +-
fs/namespace.c | 17 +++++++++--
fs/posix_acl.c | 67 ++++++++++++++++++++++++++---------------
fs/quota/quota.c | 2 +-
fs/xattr.c | 19 +++++++++---
include/linux/fs.h | 2 +-
include/linux/mount.h | 1 +
include/linux/posix_acl_xattr.h | 17 ++++++++---
include/linux/uidgid.h | 10 ++++++
include/linux/user_namespace.h | 6 ++--
kernel/capability.c | 13 +++++---
kernel/cred.c | 2 ++
kernel/user_namespace.c | 6 ++--
security/commoncap.c | 16 ++++++----
security/selinux/hooks.c | 25 ++++++++++++++-
security/smack/smack_lsm.c | 29 ++++++++++++------
29 files changed, 287 insertions(+), 109 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: I/O errors without erros from underlying device
From: Arkadiusz Miskiewicz @ 2015-12-07 20:46 UTC (permalink / raw)
To: John Stoffel; +Cc: linux-raid
In-Reply-To: <22117.49283.546268.719858@quad.stoffel.home>
On Monday 07 of December 2015, John Stoffel wrote:
> >>>>> "Arkadiusz" == Arkadiusz Miśkiewicz <arekm@maven.pl> writes:
> Arkadiusz> On Monday 07 of December 2015, John Stoffel wrote:
>
> Arkadiusz> 4.3.0 kernel, raid6 array:
> >> I think there's a bug in the 4.3.x and 4.4-rc3 and lower with block
> >> merges. I ran into these over the weekend, where v4.2.6 was stable,
> >> but anything higher would lock up and crash on me.
>
> Arkadiusz> Well, no crashes here.
>
> That's good. It was hard(er) to hit when I wasn't running KVM VMs at
> the same time on the server, and I was running strictly RAID1 disks,
> so it's hard to know.
>
> >> So first step would be to make sure you get and test v4.4-rc4.
>
> Arkadiusz> Do you know which commit there?
>
> Try this, from the master lkml git repository:
>
> 2873d32ff493ecbfb7d2c7f56812ab941dda42f4
It's merge commit. Don't see any obvious patch in that merge that would help
my case.
Anyway I would expect my problem to be related to badblock lists which numbers
are close to dmesg error message: [ 848.988518] Buffer I/O error on dev md7,
logical block 3907148544, async page read
> >> http://sprunge.us/XSWI
But how to repair these if write() also fails and
http://www.spinics.net/lists/raid/msg49325.html suggests that write should
"fix" these (by using replacement blocks I guess) ?
> Arkadiusz> md7 : active raid6 sdg[10] sdad1[9] sdac1[8] sdag1[7] sdaf1[6]
>
> >> sdae1[5] sdaj1[4] sdai1[3] sdah1[2] sdn1[1] Arkadiusz> 31255089152
> >> blocks super 1.2 level 6, 512k chunk, algorithm 2 [10/10] [UUUUUUUUUU]
>
> Arkadiusz> bitmap: 1/30 pages [4KB], 65536KB chunk
>
> Arkadiusz> array had weird failure where many disks went into failed state
>
> >> but Arkadiusz> remove && adding these disks "fixed" it (turns out not
> >> really fixed it).
>
> Arkadiusz> Unfortunately now some reads fail:
>
> Arkadiusz> pread(4, 0x1483a00, 4096, 16003680464896) = -1 EIO (Input/output
>
> >> error)
>
> Arkadiusz> To reproduce used xfs_io
> Arkadiusz> xfs_io -d -c "pread 16003680464896 4096" /dev/md7
> Arkadiusz> pread64: Input/output error
> Arkadiusz> which does pread exactly as shown above.
>
> Arkadiusz> write also fails for that area:
> Arkadiusz> xfs_io -d -c "pwrite 16003680464896 4096" /dev/md7
> Arkadiusz> pwrite64: Input/output error
>
> Arkadiusz> Note that nothing is written in dmesg when that happens.
>
> Arkadiusz> I've tried various offsets and sizes of pread and at some point
>
> >> that was logged: Arkadiusz> [ 848.988518] Buffer I/O error on dev md7,
> >> logical block 3907148544, async page read
>
> Arkadiusz> but no error from underlying devices.
>
> Arkadiusz> List of bad blocks:
> Arkadiusz> http://sprunge.us/XSWI
>
> Arkadiusz> What can I do now?
>
> Arkadiusz> (loosing data from that few sectors is acceptable if the rest
>
> >> will be readable)
>
> Arkadiusz> Thanks,
> Arkadiusz> --
> Arkadiusz> Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
> Arkadiusz> --
> Arkadiusz> To unsubscribe from this list: send the line "unsubscribe
>
> >> linux-raid" in Arkadiusz> the body of a message to
> >> majordomo@vger.kernel.org
>
> Arkadiusz> More majordomo info at
>
> >> http://vger.kernel.org/majordomo-info.html
>
> Arkadiusz> --
> Arkadiusz> Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
--
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
--
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: I/O errors without erros from underlying device
From: John Stoffel @ 2015-12-07 17:23 UTC (permalink / raw)
To: Arkadiusz Miśkiewicz; +Cc: John Stoffel, linux-raid
In-Reply-To: <201512071803.26434.arekm@maven.pl>
>>>>> "Arkadiusz" == Arkadiusz Miśkiewicz <arekm@maven.pl> writes:
Arkadiusz> On Monday 07 of December 2015, John Stoffel wrote:
Arkadiusz> 4.3.0 kernel, raid6 array:
>>
>> I think there's a bug in the 4.3.x and 4.4-rc3 and lower with block
>> merges. I ran into these over the weekend, where v4.2.6 was stable,
>> but anything higher would lock up and crash on me.
Arkadiusz> Well, no crashes here.
That's good. It was hard(er) to hit when I wasn't running KVM VMs at
the same time on the server, and I was running strictly RAID1 disks,
so it's hard to know.
>> So first step would be to make sure you get and test v4.4-rc4.
Arkadiusz> Do you know which commit there?
Try this, from the master lkml git repository:
2873d32ff493ecbfb7d2c7f56812ab941dda42f4
>>
Arkadiusz> md7 : active raid6 sdg[10] sdad1[9] sdac1[8] sdag1[7] sdaf1[6]
>> sdae1[5] sdaj1[4] sdai1[3] sdah1[2] sdn1[1] Arkadiusz> 31255089152
>> blocks super 1.2 level 6, 512k chunk, algorithm 2 [10/10] [UUUUUUUUUU]
Arkadiusz> bitmap: 1/30 pages [4KB], 65536KB chunk
>>
Arkadiusz> array had weird failure where many disks went into failed state
>> but Arkadiusz> remove && adding these disks "fixed" it (turns out not
>> really fixed it).
>>
Arkadiusz> Unfortunately now some reads fail:
>>
Arkadiusz> pread(4, 0x1483a00, 4096, 16003680464896) = -1 EIO (Input/output
>> error)
>>
Arkadiusz> To reproduce used xfs_io
Arkadiusz> xfs_io -d -c "pread 16003680464896 4096" /dev/md7
Arkadiusz> pread64: Input/output error
Arkadiusz> which does pread exactly as shown above.
>>
Arkadiusz> write also fails for that area:
Arkadiusz> xfs_io -d -c "pwrite 16003680464896 4096" /dev/md7
Arkadiusz> pwrite64: Input/output error
>>
Arkadiusz> Note that nothing is written in dmesg when that happens.
>>
Arkadiusz> I've tried various offsets and sizes of pread and at some point
>> that was logged: Arkadiusz> [ 848.988518] Buffer I/O error on dev md7,
>> logical block 3907148544, async page read
>>
Arkadiusz> but no error from underlying devices.
>>
Arkadiusz> List of bad blocks:
Arkadiusz> http://sprunge.us/XSWI
>>
Arkadiusz> What can I do now?
>>
Arkadiusz> (loosing data from that few sectors is acceptable if the rest
>> will be readable)
>>
Arkadiusz> Thanks,
Arkadiusz> --
Arkadiusz> Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
Arkadiusz> --
Arkadiusz> To unsubscribe from this list: send the line "unsubscribe
>> linux-raid" in Arkadiusz> the body of a message to
>> majordomo@vger.kernel.org
Arkadiusz> More majordomo info at
>> http://vger.kernel.org/majordomo-info.html
Arkadiusz> --
Arkadiusz> Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: I/O errors without erros from underlying device
From: Arkadiusz Miskiewicz @ 2015-12-07 17:06 UTC (permalink / raw)
To: John Stoffel; +Cc: linux-raid
In-Reply-To: <22117.46523.245486.830064@quad.stoffel.home>
On Monday 07 of December 2015, John Stoffel wrote:
> Arkadiusz> 4.3.0 kernel, raid6 array:
>
> I think there's a bug in the 4.3.x and 4.4-rc3 and lower with block
> merges. I ran into these over the weekend, where v4.2.6 was stable,
> but anything higher would lock up and crash on me.
Well, no crashes here.
> So first step would be to make sure you get and test v4.4-rc4.
Do you know which commit there?
>
> Arkadiusz> md7 : active raid6 sdg[10] sdad1[9] sdac1[8] sdag1[7] sdaf1[6]
> sdae1[5] sdaj1[4] sdai1[3] sdah1[2] sdn1[1] Arkadiusz> 31255089152
> blocks super 1.2 level 6, 512k chunk, algorithm 2 [10/10] [UUUUUUUUUU]
> Arkadiusz> bitmap: 1/30 pages [4KB], 65536KB chunk
>
> Arkadiusz> array had weird failure where many disks went into failed state
> but Arkadiusz> remove && adding these disks "fixed" it (turns out not
> really fixed it).
>
> Arkadiusz> Unfortunately now some reads fail:
>
> Arkadiusz> pread(4, 0x1483a00, 4096, 16003680464896) = -1 EIO (Input/output
> error)
>
> Arkadiusz> To reproduce used xfs_io
> Arkadiusz> xfs_io -d -c "pread 16003680464896 4096" /dev/md7
> Arkadiusz> pread64: Input/output error
> Arkadiusz> which does pread exactly as shown above.
>
> Arkadiusz> write also fails for that area:
> Arkadiusz> xfs_io -d -c "pwrite 16003680464896 4096" /dev/md7
> Arkadiusz> pwrite64: Input/output error
>
> Arkadiusz> Note that nothing is written in dmesg when that happens.
>
> Arkadiusz> I've tried various offsets and sizes of pread and at some point
> that was logged: Arkadiusz> [ 848.988518] Buffer I/O error on dev md7,
> logical block 3907148544, async page read
>
> Arkadiusz> but no error from underlying devices.
>
> Arkadiusz> List of bad blocks:
> Arkadiusz> http://sprunge.us/XSWI
>
> Arkadiusz> What can I do now?
>
> Arkadiusz> (loosing data from that few sectors is acceptable if the rest
> will be readable)
>
> Arkadiusz> Thanks,
> Arkadiusz> --
> Arkadiusz> Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
> Arkadiusz> --
> Arkadiusz> To unsubscribe from this list: send the line "unsubscribe
> linux-raid" in Arkadiusz> the body of a message to
> majordomo@vger.kernel.org
> Arkadiusz> More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: I/O errors without erros from underlying device
From: John Stoffel @ 2015-12-07 16:37 UTC (permalink / raw)
To: arekm; +Cc: linux-raid
In-Reply-To: <201512071705.27177.a.miskiewicz@gmail.com>
Arkadiusz> 4.3.0 kernel, raid6 array:
I think there's a bug in the 4.3.x and 4.4-rc3 and lower with block
merges. I ran into these over the weekend, where v4.2.6 was stable,
but anything higher would lock up and crash on me.
So first step would be to make sure you get and test v4.4-rc4.
Arkadiusz> md7 : active raid6 sdg[10] sdad1[9] sdac1[8] sdag1[7] sdaf1[6] sdae1[5] sdaj1[4] sdai1[3] sdah1[2] sdn1[1]
Arkadiusz> 31255089152 blocks super 1.2 level 6, 512k chunk, algorithm 2 [10/10] [UUUUUUUUUU]
Arkadiusz> bitmap: 1/30 pages [4KB], 65536KB chunk
Arkadiusz> array had weird failure where many disks went into failed state but
Arkadiusz> remove && adding these disks "fixed" it (turns out not really fixed it).
Arkadiusz> Unfortunately now some reads fail:
Arkadiusz> pread(4, 0x1483a00, 4096, 16003680464896) = -1 EIO (Input/output error)
Arkadiusz> To reproduce used xfs_io
Arkadiusz> xfs_io -d -c "pread 16003680464896 4096" /dev/md7
Arkadiusz> pread64: Input/output error
Arkadiusz> which does pread exactly as shown above.
Arkadiusz> write also fails for that area:
Arkadiusz> xfs_io -d -c "pwrite 16003680464896 4096" /dev/md7
Arkadiusz> pwrite64: Input/output error
Arkadiusz> Note that nothing is written in dmesg when that happens.
Arkadiusz> I've tried various offsets and sizes of pread and at some point that was logged:
Arkadiusz> [ 848.988518] Buffer I/O error on dev md7, logical block 3907148544, async page read
Arkadiusz> but no error from underlying devices.
Arkadiusz> List of bad blocks:
Arkadiusz> http://sprunge.us/XSWI
Arkadiusz> What can I do now?
Arkadiusz> (loosing data from that few sectors is acceptable if the rest will be readable)
Arkadiusz> Thanks,
Arkadiusz> --
Arkadiusz> Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
Arkadiusz> --
Arkadiusz> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
Arkadiusz> the body of a message to majordomo@vger.kernel.org
Arkadiusz> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* I/O errors without erros from underlying device
From: Arkadiusz Miskiewicz @ 2015-12-07 16:05 UTC (permalink / raw)
To: linux-raid
Hi.
4.3.0 kernel, raid6 array:
md7 : active raid6 sdg[10] sdad1[9] sdac1[8] sdag1[7] sdaf1[6] sdae1[5] sdaj1[4] sdai1[3] sdah1[2] sdn1[1]
31255089152 blocks super 1.2 level 6, 512k chunk, algorithm 2 [10/10] [UUUUUUUUUU]
bitmap: 1/30 pages [4KB], 65536KB chunk
array had weird failure where many disks went into failed state but
remove && adding these disks "fixed" it (turns out not really fixed it).
Unfortunately now some reads fail:
pread(4, 0x1483a00, 4096, 16003680464896) = -1 EIO (Input/output error)
To reproduce used xfs_io
xfs_io -d -c "pread 16003680464896 4096" /dev/md7
pread64: Input/output error
which does pread exactly as shown above.
write also fails for that area:
xfs_io -d -c "pwrite 16003680464896 4096" /dev/md7
pwrite64: Input/output error
Note that nothing is written in dmesg when that happens.
I've tried various offsets and sizes of pread and at some point that was logged:
[ 848.988518] Buffer I/O error on dev md7, logical block 3907148544, async page read
but no error from underlying devices.
List of bad blocks:
http://sprunge.us/XSWI
What can I do now?
(loosing data from that few sectors is acceptable if the rest will be readable)
Thanks,
--
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: RAID 5,6 sequential writing seems slower in newer kernels
From: Robert Kierski @ 2015-12-07 14:29 UTC (permalink / raw)
To: Dallas Clement; +Cc: Phil Turmel, linux-raid@vger.kernel.org
In-Reply-To: <CAE9DZUTF3KBGoKVOX4u3+mii5imaVUfEj8kbNjfYMP=8=pHLQA@mail.gmail.com>
I need some clarification on what data you want collected.... do you want iostat -xm collected before and after? Or at a periodic interval (1 sec)?
I'm measuring sustained performance. This means I’m runnig for a period of time -- 1 minute. If you want iostat collected on an interval, that would probably be more data than one would want posted to this mailing list.
But... yes, this is a RAID6.
I've switched to HDD's as storage as there is concern on the list that the DDR disk devices could be causing unintended behavior in RAID6. With 6 HDD's, each able to do 270 MB/s on their outer edge, I'm getting RAID6 throughput of about 1080 MB/s when I use a thread count of 4, using FIO.
-----Original Message-----
From: Dallas Clement [mailto:dallas.a.clement@gmail.com]
Sent: Friday, December 04, 2015 10:08 AM
To: Robert Kierski
Cc: Phil Turmel; linux-raid@vger.kernel.org
Subject: Re: RAID 5,6 sequential writing seems slower in newer kernels
Robert, is this with a RAID 6 array? Also, what are the actual throughput numbers you are getting? Would it be possible for you to capture the iostat -xm output and report the individual disk wMB/s and also the disk utilization for RAID 6?
^ permalink raw reply
* RE: RAID 5,6 sequential writing seems slower in newer kernels
From: Robert Kierski @ 2015-12-07 14:18 UTC (permalink / raw)
To: Shaohua Li; +Cc: Phil Turmel, Dallas Clement, linux-raid@vger.kernel.org
In-Reply-To: <20151204185155.GA3590@kernel.org>
I re-ran the test using buffered IO, with both XDD and FIO. Except that I was using XDD and FIO, the tests were essentially the same -- TC=4, QD=1, BS=1M, DIR=Write, Order=SEQ.
I've switched to using spinning disks, since several people on this list indicated that using DDR Disk devices with it's ultra low latency could cause unintended behavior in RAID6. My theoretical MAX TP is 1080 - 1620 MB/s for 6 HDD's that each have a TP of about 270 MB/s on the outer edge. My SAS infrastructure is such that I shouldn't have a problem achieving the Theoretical MAX, when testing on the outer edge.
With Buffered IO...
FIO reported a sustained TP of 2200 MB/s. As this is well above the theoretical MAX, I didn't trust it. I ran the same test with XDD. It reported a TP of 570 MB/s. When I watched the system via vmstat and iostat while running both of these tests, both tools indicated that I was doing about 570 MB/s while running both XDD and FIO. So there must be something wrong in the way FIO is calculating TP when using buffered IO.
When running both FIO and XDD using direct IO, I was able to get much better performance (about 1040 MB/s) with the above paramaters (TC=4, QD=1, BS=1M, DIR=Write, Order=SEQ). I am able to confirm that TP using vmstat and iostat.
While running with both Buffered-IO and Direct-IO, I watched the IO's in both iostat and vmstat and can confirm that there weren't any reads, which means there wasn't any Read-Modify-Write going on in either case.
As I am getting nearly the theoretical MAX, one might think there isn't a problem. However, when I increase TC to 5, my TP is only 750 MB/s. When I increase TC to 32, my TP is about 250 MB/s. While you may conclude that 32 threads is overwhelming the system, 5 threads shouldn't be overwhelming a system that has 12 core. When you consider that I get MAX TP with 4 threads, the fact that I get a 25% reduction by adding one thread is a problem.
Bob Kierski
Senior Storage Performance Engineer
Cray Inc.
380 Jackson Street
Suite 210
St. Paul, MN 55101
Tele: 651-967-9590
Fax: 651-605-9001
Cell: 651-890-7461
^ permalink raw reply
* Re: Unable to (un)-grow raid6
From: Phil Turmel @ 2015-12-06 21:21 UTC (permalink / raw)
To: David Waite; +Cc: linux-raid
In-Reply-To: <9F08EE4D-B926-4F8F-9F9D-F9C7234959B4@alkaline-solutions.com>
On 12/06/2015 04:13 PM, David Waite wrote:
>
>> On Dec 6, 2015, at 1:35 PM, Phil Turmel <philip@turmel.org> wrote:
>>
>> On 12/05/2015 10:04 PM, David Waite wrote:
>>> I’m having difficulty shrinking down a RAID6 array (md2) on a Sinology NAS. I wish to go from 13 drives to 11, and believe I need to go to 12 first to maintain operation and redundancy through the resizing process.
>>
>> No, you can go straight to 11 if you've set array-size properly. --grow
>> operations maintain redundancy throughout.
>
> I thought —grow maintains redundancy for power loss but not disk failure.
>
> Would I do this by simply marking the other drive I want to remove as failed?
No! Is that what you did with the other one?
--grow with a reduction in number leaves the unneeded drive(s) as hot
spares when it is complete. That's when you remove them. Or, if it
didn't pick the drives you wanted to remove, you can then do a --replace
operation, which also maintains redundancy throughout.
> I’ll try —array-size again. How is the array-size suggestion by mdadm calculated - the drives are not of uniform size.
You didn't post your --detail and --examine output as requested, so I
can't be specific. For parity arrays, the size of the smallest member
controls the size of the array.
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Unable to (un)-grow raid6
From: David Waite @ 2015-12-06 21:13 UTC (permalink / raw)
To: Phil Turmel; +Cc: linux-raid
In-Reply-To: <56649C04.9000601@turmel.org>
> On Dec 6, 2015, at 1:35 PM, Phil Turmel <philip@turmel.org> wrote:
>
> On 12/05/2015 10:04 PM, David Waite wrote:
>> I’m having difficulty shrinking down a RAID6 array (md2) on a Sinology NAS. I wish to go from 13 drives to 11, and believe I need to go to 12 first to maintain operation and redundancy through the resizing process.
>
> No, you can go straight to 11 if you've set array-size properly. --grow
> operations maintain redundancy throughout.
I thought —grow maintains redundancy for power loss but not disk failure.
Would I do this by simply marking the other drive I want to remove as failed?
I’ll try —array-size again. How is the array-size suggestion by mdadm calculated - the drives are not of uniform size.
-DW--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Unable to (un)-grow raid6
From: Phil Turmel @ 2015-12-06 20:35 UTC (permalink / raw)
To: David Waite, linux-raid
In-Reply-To: <465BF0F5-B5C7-4C02-8EB5-552B959B8D29@alkaline-solutions.com>
On 12/05/2015 10:04 PM, David Waite wrote:
> I’m having difficulty shrinking down a RAID6 array (md2) on a Sinology NAS. I wish to go from 13 drives to 11, and believe I need to go to 12 first to maintain operation and redundancy through the resizing process.
No, you can go straight to 11 if you've set array-size properly. --grow
operations maintain redundancy throughout.
> The -array-size has already been shrunk to account for the two drives I wish to remove, and one of the devices has been removed and the machine restarted before the command output below was generated.
You might need to set array-size again. It is a temporary setting that
only becomes permanent when --grow successfully starts.
> # cat /proc/mdstat
> Personalities : [linear] [raid0] [raid1] [raid10] [raid6] [raid5] [raid4]
> md2 : active raid6 sda3[0] sdia3[14] sdib3[9] sdic3[10] sdid3[11] sdie3[12] sdg3[17] sdf3[5] sde3[16] sdd3[3] sdc3[13] sdb3[15]
> 26329895935 blocks super 1.2 level 6, 64k chunk, algorithm 2 [13/12] [UUUUUUU_UUUUU]
>
> md1 : active raid1 sda2[0] sdb2[1] sdc2[2] sdd2[3] sde2[4] sdf2[5] sdg2[6]
> 2097088 blocks [8/7] [UUUUUUU_]
>
> md0 : active raid1 sda1[0] sdb1[1] sdc1[2] sdd1[3] sde1[4] sdf1[5] sdg1[6]
> 2490176 blocks [8/7] [UUUUUUU_]
>
> unused devices: <none>
> When running
> # mdadm --grow -n 12 /dev/md2 --backup-file=/mnt/backup-file3
> mdadm: max_devs [384] of [/dev/md2]
> mdadm: Need to backup 7040K of critical section..
> mdadm: Cannot set device shape for /dev/md2: Invalid argument
>
> ---
>
> dmesg just reports:
> [87908.606245] md: couldn't update array info. -22
Error code 22 is "Invalid Argument", corresponding to the mdadm output.
Try setting array size again, then try again. Feel free to go straight
to 11 devices -- it'll save you much time.
Also, for completeness, show --detail and --examine for the array and it
members.
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Unable to (un)-grow raid6
From: David Waite @ 2015-12-06 3:04 UTC (permalink / raw)
To: linux-raid
In-Reply-To: <4E26C335-DEDB-489E-B54E-A285273569A1@alkaline-solutions.com>
I’m having difficulty shrinking down a RAID6 array (md2) on a Sinology NAS. I wish to go from 13 drives to 11, and believe I need to go to 12 first to maintain operation and redundancy through the resizing process.
The -array-size has already been shrunk to account for the two drives I wish to remove, and one of the devices has been removed and the machine restarted before the command output below was generated.
---
# cat /proc/mdstat
Personalities : [linear] [raid0] [raid1] [raid10] [raid6] [raid5] [raid4]
md2 : active raid6 sda3[0] sdia3[14] sdib3[9] sdic3[10] sdid3[11] sdie3[12] sdg3[17] sdf3[5] sde3[16] sdd3[3] sdc3[13] sdb3[15]
26329895935 blocks super 1.2 level 6, 64k chunk, algorithm 2 [13/12] [UUUUUUU_UUUUU]
md1 : active raid1 sda2[0] sdb2[1] sdc2[2] sdd2[3] sde2[4] sdf2[5] sdg2[6]
2097088 blocks [8/7] [UUUUUUU_]
md0 : active raid1 sda1[0] sdb1[1] sdc1[2] sdd1[3] sde1[4] sdf1[5] sdg1[6]
2490176 blocks [8/7] [UUUUUUU_]
unused devices: <none>
---
When running
# mdadm --grow -n 12 /dev/md2 --backup-file=/mnt/backup-file3
mdadm: max_devs [384] of [/dev/md2]
mdadm: Need to backup 7040K of critical section..
mdadm: Cannot set device shape for /dev/md2: Invalid argument
---
dmesg just reports:
[87908.606245] md: couldn't update array info. -22
---
# uname -a
Linux diskstation 3.10.77 #7135 SMP Thu Oct 15 13:36:56 CST 2015 x86_64 GNU/Linux synology_avoton_1815+
---
Anyone have advice on how to proceed? I thought shrinking of a RAID6 array is supported as long as you use a backup file to allow recovery of the initial blocks
-DW
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Seth Forshee @ 2015-12-05 6:15 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Theodore Ts'o, Serge E. Hallyn, Eric W. Biederman,
Alexander Viro, Serge Hallyn, Richard Weinberger,
Austin S Hemmelgarn, Miklos Szeredi, linux-bcache, dm-devel,
linux-raid, linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <20151204234348.GA6908@mail.hallyn.com>
On Fri, Dec 04, 2015 at 05:43:49PM -0600, Serge E. Hallyn wrote:
> On Fri, Dec 04, 2015 at 06:11:52PM -0500, Theodore Ts'o wrote:
> > On Fri, Dec 04, 2015 at 02:45:32PM -0600, Seth Forshee wrote:
> > > On Fri, Dec 04, 2015 at 02:07:36PM -0600, Serge E. Hallyn wrote:
> > > > Heh, I was looking over http://www.gossamer-threads.com/lists/linux/kernel/103611
> > > > a little while ago :) The same question was asked 16 years ago. Apparently
> > > > the answer then was that it was easier than fixing the code.
> > >
> > > So it seems then that either it still isn't safe and so unprivileged
> > > users shouldn't be allowed to do it at all, or else it's safe and we
> > > should drop the requirement completely. I can't say which is right,
> > > unfortunately.
> >
> > It may not have been safe 16 years agoo, but giving invalid arguments
> > to FIBMAP is safe for ext4 and ext2. This is the sort of thing that
> > tools like trinity should and does test for, so I think it should be
> > fine to remove the root check for FIBMAP.
>
> Seth, can I tempt you into sending a standalone patch to remove that? :)
Patch sent. I'll drop this patch in v2.
^ permalink raw reply
* Re: RAID 5,6 sequential writing seems slower in newer kernels
From: Dallas Clement @ 2015-12-05 1:38 UTC (permalink / raw)
To: Shaohua Li; +Cc: Robert Kierski, Phil Turmel, linux-raid@vger.kernel.org
In-Reply-To: <20151204185155.GA3590@kernel.org>
On Fri, Dec 4, 2015 at 12:51 PM, Shaohua Li <shli@kernel.org> wrote:
> On Fri, Dec 04, 2015 at 01:40:02PM +0000, Robert Kierski wrote:
>> It turns out the problem I'm experiencing is related to thread count. When I run XDD with a reasonable queuedepth parameter (32), I get horrible performance. When I run it with a small queuedepth (1-4), I get expected performance.
>>
>> Here are the command lines:
>>
>> Horrible Performance:
>> xdd -id commandline -dio -maxall -targets 1 /dev/md0 -queuedepth 32 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>>
>> GOOD Performance:
>> xdd -id commandline -dio -maxall -targets 1 /dev/md0 -queuedepth 1 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>>
>> BEST Performance:
>> xdd -id commandline -dio -maxall -targets 1 /dev/md0 -queuedepth 3 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>>
>> BAD Performance
>> xdd -id commandline -dio -maxall -targets 1 /dev/md1 -queuedepth 5 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>
> the performance issue only happens for directIO write, right? did you check
> buffered write? The directIO case doesn't delay write, so will create more
> read-modify-write. you can check with below debug code.
>
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 45933c1..d480cc3 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5278,10 +5278,10 @@ static void make_request(struct mddev *mddev, struct bio * bi)
> }
> set_bit(STRIPE_HANDLE, &sh->state);
> clear_bit(STRIPE_DELAYED, &sh->state);
> - if ((!sh->batch_head || sh == sh->batch_head) &&
> - (bi->bi_rw & REQ_SYNC) &&
> - !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> - atomic_inc(&conf->preread_active_stripes);
> +// if ((!sh->batch_head || sh == sh->batch_head) &&
> +// (bi->bi_rw & REQ_SYNC) &&
> +// !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> +// atomic_inc(&conf->preread_active_stripes);
> release_stripe_plug(mddev, sh);
> } else {
> /* cannot get stripe for read-ahead, just give-up */
Hi all. My original test involved fio sequential writing to XFS
formatted RAID devices with block size = 2M and queue depth = 256.
Today I spent some time focused on testing raw RAID sequential write
tests with dd similar to Robert's tests. I am happy to report that I
see the exact opposite results that I reported earlier with fio / XFS.
When comparing performance between the 2.6.39.4 kernel and the 3.10.69
kernel, I am seeing that RAID 0 and and RAID 1 write speeds are about
the same. However, RAID 5 is about 60% faster in the 3.10.69 kernel
and RAID 6 is 40% faster. I am not sure how to control queue depth
with plain old dd.
Next I am going to get a second opinion from fio, this time writing
directly to the RAID devices instead of going through XFS with varying
queue depth. If I see the same behavior as with DD, then there is no
problem with RAID in the new kernels - it is something else, perhaps
XFS. Will report my fio findings as soon as I have a chance to
capture them.
^ permalink raw reply
* Re: [PATCH v2 2/3] block: Add badblock management for gendisks
From: Verma, Vishal L @ 2015-12-05 0:17 UTC (permalink / raw)
To: James.Bottomley@HansenPartnership.com
Cc: linux-raid@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-nvdimm@lists.01.org, linux-block@vger.kernel.org,
neilb@suse.com, axboe@fb.com, jmoyer@redhat.com
In-Reply-To: <1449272018.27077.29.camel@HansenPartnership.com>
On Fri, 2015-12-04 at 15:33 -0800, James Bottomley wrote:
[...]
> > static void register_disk(struct gendisk *disk)
> > {
> > struct device *ddev = disk_to_dev(disk);
> > @@ -609,6 +624,7 @@ void add_disk(struct gendisk *disk)
> > disk->first_minor = MINOR(devt);
> >
> > disk_alloc_events(disk);
> > + disk_alloc_badblocks(disk);
>
> Why unconditionally do this? No-one currently uses the interface, but
> every disk will now pay the price of an additional structure plus a
> page
> for no benefit. You should probably either export the initializer for
> those who want to use it or, perhaps even better, make it lazily
> allocated the first time anyone tries to set a bad block.
>
> If you come up with a really good reason for allocating it
> unconditionally, then it should probably be an embedded structure in
> the gendisk.
>
Agreed - I'll fix for v3.
I'm considering an embedded structure in gendisk (same as md) (why is
this preferred to pointer chasing, especially when this wastes more
space?), and a new exported initializer that is used by anyone who wants
to use gendisk's badblocks.
-Vishal
^ permalink raw reply
* Re: [PATCH v2 1/3] badblocks: Add core badblock management code
From: Verma, Vishal L @ 2015-12-05 0:11 UTC (permalink / raw)
To: James.Bottomley@HansenPartnership.com
Cc: linux-raid@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-nvdimm@lists.01.org, neilb@suse.com,
linux-block@vger.kernel.org, jmoyer@redhat.com, axboe@fb.com
In-Reply-To: <1449273995.27077.32.camel@HansenPartnership.com>
On Fri, 2015-12-04 at 16:06 -0800, James Bottomley wrote:
> On Fri, 2015-12-04 at 23:58 +0000, Verma, Vishal L wrote:
> > On Fri, 2015-12-04 at 15:30 -0800, James Bottomley wrote:
> > [...]
> > > > + * We return
> > > > + * 0 if there are no known bad blocks in the range
> > > > + * 1 if there are known bad block which are all acknowledged
> > > > + * -1 if there are bad blocks which have not yet been
> > > > acknowledged
> > > > in metadata.
> > > > + * plus the start/length of the first bad section we overlap.
> > > > + */
> > >
> > > This comment should be docbook.
> >
> > Applicable to all your comments - (and they are all valid), I simply
> > copied over all this from md. I'm happy to make the changes to
> > comments,
> > and the other two things (see below) if that's the right thing to do
> > --
> > I just tried to keep my own changes to the original md badblocks
> > code
> > minimal.
> > Would it be better (for review-ability) if I made these changes in a
> > new
> > patch on top of this, or should I just squash them into this one?
>
> If you were moving it, that might be appropriate. However, this is
> effectively new code because you're not removing the original, so we
> should begin at least with a coherent API. (i.e. corrections to the
> original patch rather than incremental).
>
Patch 3 does remove the original code, but yes, I agree. Will send
another version.
Thanks for the review.
-Vishal
^ permalink raw reply
* Re: [PATCH v2 1/3] badblocks: Add core badblock management code
From: James Bottomley @ 2015-12-05 0:06 UTC (permalink / raw)
To: Verma, Vishal L
Cc: neilb@suse.com, linux-raid@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-nvdimm@lists.01.org,
linux-block@vger.kernel.org, jmoyer@redhat.com, axboe@fb.com
In-Reply-To: <1449273524.16905.103.camel@intel.com>
On Fri, 2015-12-04 at 23:58 +0000, Verma, Vishal L wrote:
> On Fri, 2015-12-04 at 15:30 -0800, James Bottomley wrote:
> [...]
> > > + * We return
> > > + * 0 if there are no known bad blocks in the range
> > > + * 1 if there are known bad block which are all acknowledged
> > > + * -1 if there are bad blocks which have not yet been acknowledged
> > > in metadata.
> > > + * plus the start/length of the first bad section we overlap.
> > > + */
> >
> > This comment should be docbook.
>
> Applicable to all your comments - (and they are all valid), I simply
> copied over all this from md. I'm happy to make the changes to comments,
> and the other two things (see below) if that's the right thing to do --
> I just tried to keep my own changes to the original md badblocks code
> minimal.
> Would it be better (for review-ability) if I made these changes in a new
> patch on top of this, or should I just squash them into this one?
If you were moving it, that might be appropriate. However, this is
effectively new code because you're not removing the original, so we
should begin at least with a coherent API. (i.e. corrections to the
original patch rather than incremental).
Thanks,
James
> >
> > > +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> > > + sector_t *first_bad, int *bad_sectors)
> > [...]
> > > +
> > > +/*
> > > + * Add a range of bad blocks to the table.
> > > + * This might extend the table, or might contract it
> > > + * if two adjacent ranges can be merged.
> > > + * We binary-search to find the 'insertion' point, then
> > > + * decide how best to handle it.
> > > + */
> >
> > And this one, plus you don't document returns. It looks like this
> > function returns 1 on success and zero on failure, which is really
> > counter-intuitive for the kernel: zero is usually returned on success
> > and negative error on failure.
> >
> > > +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> > > + int acknowledged)
> > [...]
> > > +
> > > +/*
> > > + * Remove a range of bad blocks from the table.
> > > + * This may involve extending the table if we spilt a region,
> > > + * but it must not fail. So if the table becomes full, we just
> > > + * drop the remove request.
> > > + */
> >
> > Docbook and document returns. This time they're the kernel standard
> > of
> > 0 on success and negative error on failure making the convention for
> > badblocks_set even more counterintuitive.
> >
> > > +int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> > > +{
> > [...]
> > > +#define DO_DEBUG 1
> >
> > Why have this at all if it's unconditionally defined and always set.
>
> Neil - any reason or anything you had in mind for this? Or is it just an
> artifact and can be removed.
>
> >
> > > +ssize_t badblocks_store(struct badblocks *bb, const char *page,
> > > size_t len,
> > > + int unack)
> > [...]
> > > +int badblocks_init(struct badblocks *bb, int enable)
> > > +{
> > > + bb->count = 0;
> > > + if (enable)
> > > + bb->shift = 0;
> > > + else
> > > + bb->shift = -1;
> > > + bb->page = kmalloc(PAGE_SIZE, GFP_KERNEL);
> >
> > Why not __get_free_page(GFP_KERNEL)? The problem with kmalloc of an
> > exactly known page sized quantity is that the slab tracker for this
> > requires two contiguous pages for each page because of the overhead.
>
> Cool, I didn't know about __get_free_page - I can fix this up too.
>
> >
> > James
> >
> > NrybXǧv^){.n+{"{ay\x1dʇڙ,j\afhz\x1ew\fj:+vwjm\azZ+ݢj"!
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Andreas Dilger @ 2015-12-05 0:00 UTC (permalink / raw)
To: Seth Forshee, Serge E. Hallyn
Cc: Theodore Ts'o, Eric W. Biederman, Alexander Viro,
Serge Hallyn, Richard Weinberger, Austin S Hemmelgarn,
Miklos Szeredi, linux-bcache, dm-devel, linux-raid, LKML,
linux-mtd, linux-fsdevel, fuse-devel, linux-security-module,
selinux
In-Reply-To: <20151204231152.GE18359@thunk.org>
[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]
> On Dec 4, 2015, at 4:11 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Dec 04, 2015 at 02:45:32PM -0600, Seth Forshee wrote:
>> On Fri, Dec 04, 2015 at 02:07:36PM -0600, Serge E. Hallyn wrote:
>>> Heh, I was looking over http://www.gossamer-threads.com/lists/linux/kernel/103611
>>> a little while ago :) The same question was asked 16 years ago. Apparently
>>> the answer then was that it was easier than fixing the code.
>>
>> So it seems then that either it still isn't safe and so unprivileged
>> users shouldn't be allowed to do it at all, or else it's safe and we
>> should drop the requirement completely. I can't say which is right,
>> unfortunately.
>
> It may not have been safe 16 years agoo, but giving invalid arguments
> to FIBMAP is safe for ext4 and ext2. This is the sort of thing that
> tools like trinity should and does test for, so I think it should be
> fine to remove the root check for FIBMAP.
You can use FIEMAP on regular files and directories without special permission:
$ filefrag -v /etc
Filesystem type is: ef53
File size of /etc is 12288 (3 blocks of 4096 bytes)
ext: logical_offset: physical_offset: length: expected: flags:
0: 0.. 0: 8396832.. 8396832: 1:
1: 1.. 2: 8397051.. 8397052: 2: 8396833: last,eof
/etc: 2 extents found
FIEMAP also has the benefit that you don't need to call it millions of times
for large files, like is needed for FIBMAP.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] badblocks: Add core badblock management code
From: Verma, Vishal L @ 2015-12-04 23:58 UTC (permalink / raw)
To: James.Bottomley@HansenPartnership.com, neilb@suse.com
Cc: linux-raid@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-nvdimm@lists.01.org, linux-block@vger.kernel.org,
jmoyer@redhat.com, axboe@fb.com
In-Reply-To: <1449271801.27077.25.camel@HansenPartnership.com>
On Fri, 2015-12-04 at 15:30 -0800, James Bottomley wrote:
[...]
> > + * We return
> > + * 0 if there are no known bad blocks in the range
> > + * 1 if there are known bad block which are all acknowledged
> > + * -1 if there are bad blocks which have not yet been acknowledged
> > in metadata.
> > + * plus the start/length of the first bad section we overlap.
> > + */
>
> This comment should be docbook.
Applicable to all your comments - (and they are all valid), I simply
copied over all this from md. I'm happy to make the changes to comments,
and the other two things (see below) if that's the right thing to do --
I just tried to keep my own changes to the original md badblocks code
minimal.
Would it be better (for review-ability) if I made these changes in a new
patch on top of this, or should I just squash them into this one?
>
> > +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> > + sector_t *first_bad, int *bad_sectors)
> [...]
> > +
> > +/*
> > + * Add a range of bad blocks to the table.
> > + * This might extend the table, or might contract it
> > + * if two adjacent ranges can be merged.
> > + * We binary-search to find the 'insertion' point, then
> > + * decide how best to handle it.
> > + */
>
> And this one, plus you don't document returns. It looks like this
> function returns 1 on success and zero on failure, which is really
> counter-intuitive for the kernel: zero is usually returned on success
> and negative error on failure.
>
> > +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> > + int acknowledged)
> [...]
> > +
> > +/*
> > + * Remove a range of bad blocks from the table.
> > + * This may involve extending the table if we spilt a region,
> > + * but it must not fail. So if the table becomes full, we just
> > + * drop the remove request.
> > + */
>
> Docbook and document returns. This time they're the kernel standard
> of
> 0 on success and negative error on failure making the convention for
> badblocks_set even more counterintuitive.
>
> > +int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> > +{
> [...]
> > +#define DO_DEBUG 1
>
> Why have this at all if it's unconditionally defined and always set.
Neil - any reason or anything you had in mind for this? Or is it just an
artifact and can be removed.
>
> > +ssize_t badblocks_store(struct badblocks *bb, const char *page,
> > size_t len,
> > + int unack)
> [...]
> > +int badblocks_init(struct badblocks *bb, int enable)
> > +{
> > + bb->count = 0;
> > + if (enable)
> > + bb->shift = 0;
> > + else
> > + bb->shift = -1;
> > + bb->page = kmalloc(PAGE_SIZE, GFP_KERNEL);
>
> Why not __get_free_page(GFP_KERNEL)? The problem with kmalloc of an
> exactly known page sized quantity is that the slab tracker for this
> requires two contiguous pages for each page because of the overhead.
Cool, I didn't know about __get_free_page - I can fix this up too.
>
> James
>
>
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Serge E. Hallyn @ 2015-12-04 23:43 UTC (permalink / raw)
To: Theodore Ts'o, Seth Forshee, Serge E. Hallyn,
Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204231152.GE18359@thunk.org>
On Fri, Dec 04, 2015 at 06:11:52PM -0500, Theodore Ts'o wrote:
> On Fri, Dec 04, 2015 at 02:45:32PM -0600, Seth Forshee wrote:
> > On Fri, Dec 04, 2015 at 02:07:36PM -0600, Serge E. Hallyn wrote:
> > > Heh, I was looking over http://www.gossamer-threads.com/lists/linux/kernel/103611
> > > a little while ago :) The same question was asked 16 years ago. Apparently
> > > the answer then was that it was easier than fixing the code.
> >
> > So it seems then that either it still isn't safe and so unprivileged
> > users shouldn't be allowed to do it at all, or else it's safe and we
> > should drop the requirement completely. I can't say which is right,
> > unfortunately.
>
> It may not have been safe 16 years agoo, but giving invalid arguments
> to FIBMAP is safe for ext4 and ext2. This is the sort of thing that
> tools like trinity should and does test for, so I think it should be
> fine to remove the root check for FIBMAP.
Seth, can I tempt you into sending a standalone patch to remove that? :)
-serge
^ permalink raw reply
* Re: [PATCH v2 2/3] block: Add badblock management for gendisks
From: James Bottomley @ 2015-12-04 23:33 UTC (permalink / raw)
To: Vishal Verma
Cc: linux-nvdimm, linux-block, linux-raid, linux-scsi, Jens Axboe,
NeilBrown, Jeff Moyer
In-Reply-To: <1448477013-9174-3-git-send-email-vishal.l.verma@intel.com>
On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> NVDIMM devices, which can behave more like DRAM rather than block
> devices, may develop bad cache lines, or 'poison'. A block device
> exposed by the pmem driver can then consume poison via a read (or
> write), and cause a machine check. On platforms without machine
> check recovery features, this would mean a crash.
>
> The block device maintaining a runtime list of all known sectors that
> have poison can directly avoid this, and also provide a path forward
> to enable proper handling/recovery for DAX faults on such a device.
>
> Use the new badblock management interfaces to add a badblocks list to
> gendisks.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
> block/genhd.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/genhd.h | 6 ++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/block/genhd.c b/block/genhd.c
> index 0c706f3..84fd65c 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -20,6 +20,7 @@
> #include <linux/idr.h>
> #include <linux/log2.h>
> #include <linux/pm_runtime.h>
> +#include <linux/badblocks.h>
>
> #include "blk.h"
>
> @@ -505,6 +506,20 @@ static int exact_lock(dev_t devt, void *data)
> return 0;
> }
>
> +static void disk_alloc_badblocks(struct gendisk *disk)
> +{
> + disk->bb = kzalloc(sizeof(*(disk->bb)), GFP_KERNEL);
> + if (!disk->bb) {
> + pr_warn("%s: failed to allocate space for badblocks\n",
> + disk->disk_name);
> + return;
> + }
> +
> + if (badblocks_init(disk->bb, 1))
> + pr_warn("%s: failed to initialize badblocks\n",
> + disk->disk_name);
> +}
> +
> static void register_disk(struct gendisk *disk)
> {
> struct device *ddev = disk_to_dev(disk);
> @@ -609,6 +624,7 @@ void add_disk(struct gendisk *disk)
> disk->first_minor = MINOR(devt);
>
> disk_alloc_events(disk);
> + disk_alloc_badblocks(disk);
Why unconditionally do this? No-one currently uses the interface, but
every disk will now pay the price of an additional structure plus a page
for no benefit. You should probably either export the initializer for
those who want to use it or, perhaps even better, make it lazily
allocated the first time anyone tries to set a bad block.
If you come up with a really good reason for allocating it
unconditionally, then it should probably be an embedded structure in the
gendisk.
James
^ permalink raw reply
* Re: [PATCH v2 1/3] badblocks: Add core badblock management code
From: James Bottomley @ 2015-12-04 23:30 UTC (permalink / raw)
To: Vishal Verma
Cc: linux-nvdimm, linux-block, linux-raid, linux-scsi, Jens Axboe,
NeilBrown, Jeff Moyer
In-Reply-To: <1448477013-9174-2-git-send-email-vishal.l.verma@intel.com>
On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> Take the core badblocks implementation from md, and make it generally
> available. This follows the same style as kernel implementations of
> linked lists, rb-trees etc, where you can have a structure that can be
> embedded anywhere, and accessor functions to manipulate the data.
>
> The only changes in this copy of the code are ones to generalize
> function/variable names from md-specific ones. Also add init and free
> functions.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
> block/Makefile | 2 +-
> block/badblocks.c | 523 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/badblocks.h | 53 +++++
> 3 files changed, 577 insertions(+), 1 deletion(-)
> create mode 100644 block/badblocks.c
> create mode 100644 include/linux/badblocks.h
>
> diff --git a/block/Makefile b/block/Makefile
> index 00ecc97..db5f622 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -8,7 +8,7 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-tag.o blk-sysfs.o \
> blk-iopoll.o blk-lib.o blk-mq.o blk-mq-tag.o \
> blk-mq-sysfs.o blk-mq-cpu.o blk-mq-cpumap.o ioctl.o \
> genhd.o scsi_ioctl.o partition-generic.o ioprio.o \
> - partitions/
> + badblocks.o partitions/
>
> obj-$(CONFIG_BOUNCE) += bounce.o
> obj-$(CONFIG_BLK_DEV_BSG) += bsg.o
> diff --git a/block/badblocks.c b/block/badblocks.c
> new file mode 100644
> index 0000000..6e07855
> --- /dev/null
> +++ b/block/badblocks.c
> @@ -0,0 +1,523 @@
> +/*
> + * Bad block management
> + *
> + * - Heavily based on MD badblocks code from Neil Brown
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/badblocks.h>
> +#include <linux/seqlock.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +
> +/*
> + * We can record which blocks on each device are 'bad' and so just
> + * fail those blocks, or that stripe, rather than the whole device.
> + * Entries in the bad-block table are 64bits wide. This comprises:
> + * Length of bad-range, in sectors: 0-511 for lengths 1-512
> + * Start of bad-range, sector offset, 54 bits (allows 8 exbibytes)
> + * A 'shift' can be set so that larger blocks are tracked and
> + * consequently larger devices can be covered.
> + * 'Acknowledged' flag - 1 bit. - the most significant bit.
> + *
> + * Locking of the bad-block table uses a seqlock so badblocks_check
> + * might need to retry if it is very unlucky.
> + * We will sometimes want to check for bad blocks in a bi_end_io function,
> + * so we use the write_seqlock_irq variant.
> + *
> + * When looking for a bad block we specify a range and want to
> + * know if any block in the range is bad. So we binary-search
> + * to the last range that starts at-or-before the given endpoint,
> + * (or "before the sector after the target range")
> + * then see if it ends after the given start.
> + * We return
> + * 0 if there are no known bad blocks in the range
> + * 1 if there are known bad block which are all acknowledged
> + * -1 if there are bad blocks which have not yet been acknowledged in metadata.
> + * plus the start/length of the first bad section we overlap.
> + */
This comment should be docbook.
> +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> + sector_t *first_bad, int *bad_sectors)
[...]
> +
> +/*
> + * Add a range of bad blocks to the table.
> + * This might extend the table, or might contract it
> + * if two adjacent ranges can be merged.
> + * We binary-search to find the 'insertion' point, then
> + * decide how best to handle it.
> + */
And this one, plus you don't document returns. It looks like this
function returns 1 on success and zero on failure, which is really
counter-intuitive for the kernel: zero is usually returned on success
and negative error on failure.
> +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> + int acknowledged)
[...]
> +
> +/*
> + * Remove a range of bad blocks from the table.
> + * This may involve extending the table if we spilt a region,
> + * but it must not fail. So if the table becomes full, we just
> + * drop the remove request.
> + */
Docbook and document returns. This time they're the kernel standard of
0 on success and negative error on failure making the convention for
badblocks_set even more counterintuitive.
> +int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +{
[...]
> +#define DO_DEBUG 1
Why have this at all if it's unconditionally defined and always set.
> +ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
> + int unack)
[...]
> +int badblocks_init(struct badblocks *bb, int enable)
> +{
> + bb->count = 0;
> + if (enable)
> + bb->shift = 0;
> + else
> + bb->shift = -1;
> + bb->page = kmalloc(PAGE_SIZE, GFP_KERNEL);
Why not __get_free_page(GFP_KERNEL)? The problem with kmalloc of an
exactly known page sized quantity is that the slab tracker for this
requires two contiguous pages for each page because of the overhead.
James
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox