* [PATCH 13/16] block: remove blk_part_pack_uuid
From: Christoph Hellwig @ 2017-05-10 18:02 UTC (permalink / raw)
To: Andy Shevchenko, Amir Goldstein,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Howells, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-1-hch-jcswGhMUV9g@public.gmane.org>
This helper was only used by IMA of all things, which would get spurious
errors if CONFIG_BLOCK is disabled. Just opencode the call there.
Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
include/linux/genhd.h | 11 -----------
security/integrity/ima/ima_policy.c | 6 ++----
2 files changed, 2 insertions(+), 15 deletions(-)
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index acff9437e5c3..e619fae2f037 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -219,12 +219,6 @@ static inline struct gendisk *part_to_disk(struct hd_struct *part)
return NULL;
}
-static inline int blk_part_pack_uuid(const u8 *uuid_str, u8 *to)
-{
- uuid_be_to_bin(uuid_str, (uuid_be *)to);
- return 0;
-}
-
static inline int disk_max_parts(struct gendisk *disk)
{
if (disk->flags & GENHD_FL_EXT_DEVT)
@@ -736,11 +730,6 @@ static inline dev_t blk_lookup_devt(const char *name, int partno)
dev_t devt = MKDEV(0, 0);
return devt;
}
-
-static inline int blk_part_pack_uuid(const u8 *uuid_str, u8 *to)
-{
- return -EINVAL;
-}
#endif /* CONFIG_BLOCK */
#endif /* _LINUX_GENHD_H */
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 3ab1067db624..4d1178610145 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -717,10 +717,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
}
- result = blk_part_pack_uuid(args[0].from,
- entry->fsuuid);
- if (!result)
- entry->flags |= IMA_FSUUID;
+ uuid_to_bin(args[0].from, (uuid_t *)entry->fsuuid);
+ entry->flags |= IMA_FSUUID;
break;
case Opt_uid_gt:
case Opt_euid_gt:
--
2.11.0
^ permalink raw reply related
* [PATCH 14/16] block: remove blk_part_pack_uuid
From: Christoph Hellwig @ 2017-05-10 18:02 UTC (permalink / raw)
To: Andy Shevchenko, Amir Goldstein,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Howells, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-1-hch-jcswGhMUV9g@public.gmane.org>
This helper was only used by IMA of all things, which would get spurious
errors if CONFIG_BLOCK is disabled. Just opencode the call there.
Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
[andy: correctly handle error returns from uuid_to_bin]
Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
security/integrity/ima/ima_policy.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4d1178610145..49fbc3e8f012 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -717,8 +717,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
}
- uuid_to_bin(args[0].from, (uuid_t *)entry->fsuuid);
- entry->flags |= IMA_FSUUID;
+ result = uuid_to_bin(args[0].from, (uuid_t *)&entry->fsuuid);
+ if (!result)
+ entry->flags |= IMA_FSUUID;
break;
case Opt_uid_gt:
case Opt_euid_gt:
--
2.11.0
^ permalink raw reply related
* [PATCH 15/16] ima/policy: switch to use uuid_t
From: Christoph Hellwig @ 2017-05-10 18:02 UTC (permalink / raw)
To: Andy Shevchenko, Amir Goldstein,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Howells, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-1-hch-jcswGhMUV9g@public.gmane.org>
Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
security/integrity/ima/ima_policy.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 49fbc3e8f012..da3e7d50e0d7 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -61,7 +61,7 @@ struct ima_rule_entry {
enum ima_hooks func;
int mask;
unsigned long fsmagic;
- u8 fsuuid[16];
+ uuid_t fsuuid;
kuid_t uid;
kuid_t fowner;
bool (*uid_op)(kuid_t, kuid_t); /* Handlers for operators */
@@ -244,7 +244,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
&& rule->fsmagic != inode->i_sb->s_magic)
return false;
if ((rule->flags & IMA_FSUUID) &&
- memcmp(rule->fsuuid, inode->i_sb->s_uuid, sizeof(rule->fsuuid)))
+ memcmp(&rule->fsuuid, inode->i_sb->s_uuid, sizeof(rule->fsuuid)))
return false;
if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
return false;
@@ -711,13 +711,12 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
case Opt_fsuuid:
ima_log_string(ab, "fsuuid", args[0].from);
- if (memchr_inv(entry->fsuuid, 0x00,
- sizeof(entry->fsuuid))) {
+ if (uuid_is_null(&entry->fsuuid)) {
result = -EINVAL;
break;
}
- result = uuid_to_bin(args[0].from, (uuid_t *)&entry->fsuuid);
+ result = uuid_to_bin(args[0].from, &entry->fsuuid);
if (!result)
entry->flags |= IMA_FSUUID;
break;
@@ -1086,7 +1085,7 @@ int ima_policy_show(struct seq_file *m, void *v)
}
if (entry->flags & IMA_FSUUID) {
- seq_printf(m, "fsuuid=%pU", entry->fsuuid);
+ seq_printf(m, "fsuuid=%pU", &entry->fsuuid);
seq_puts(m, " ");
}
--
2.11.0
^ permalink raw reply related
* [PATCH 16/16] fs: switch ->s_uuid to uuid_t
From: Christoph Hellwig @ 2017-05-10 18:02 UTC (permalink / raw)
To: Andy Shevchenko, Amir Goldstein,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Howells, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-1-hch-jcswGhMUV9g@public.gmane.org>
For some file systems we still memcpy into it, but in various places this
already allows us to use the proper uuid helpers. More to come..
Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
drivers/xen/tmem.c | 6 +++---
fs/ext4/super.c | 2 +-
fs/f2fs/super.c | 2 +-
fs/gfs2/ops_fstype.c | 2 +-
fs/gfs2/sys.c | 22 +++++-----------------
fs/ocfs2/super.c | 2 +-
fs/xfs/xfs_mount.c | 2 +-
include/linux/cleancache.h | 2 +-
include/linux/fs.h | 5 +++--
mm/cleancache.c | 2 +-
security/integrity/evm/evm_crypto.c | 2 +-
security/integrity/ima/ima_policy.c | 2 +-
12 files changed, 20 insertions(+), 31 deletions(-)
diff --git a/drivers/xen/tmem.c b/drivers/xen/tmem.c
index 4ac2ca8a7656..bf13d1ec51f3 100644
--- a/drivers/xen/tmem.c
+++ b/drivers/xen/tmem.c
@@ -233,12 +233,12 @@ static int tmem_cleancache_init_fs(size_t pagesize)
return xen_tmem_new_pool(uuid_private, 0, pagesize);
}
-static int tmem_cleancache_init_shared_fs(char *uuid, size_t pagesize)
+static int tmem_cleancache_init_shared_fs(uuid_t *uuid, size_t pagesize)
{
struct tmem_pool_uuid shared_uuid;
- shared_uuid.uuid_lo = *(u64 *)uuid;
- shared_uuid.uuid_hi = *(u64 *)(&uuid[8]);
+ shared_uuid.uuid_lo = *(u64 *)&uuid->b[0];
+ shared_uuid.uuid_hi = *(u64 *)&uuid->b[8];
return xen_tmem_new_pool(shared_uuid, TMEM_POOL_SHARED, pagesize);
}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c90edf09b0c3..b53e8b3082c0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3951,7 +3951,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
sb->s_qcop = &ext4_qctl_operations;
sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP | QTYPE_MASK_PRJ;
#endif
- memcpy(sb->s_uuid, es->s_uuid, sizeof(es->s_uuid));
+ memcpy(&sb->s_uuid, es->s_uuid, sizeof(es->s_uuid));
INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
mutex_init(&sbi->s_orphan_lock);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 83355ec4a92c..0b89b0b7b9f7 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1937,7 +1937,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_time_gran = 1;
sb->s_flags = (sb->s_flags & ~MS_POSIXACL) |
(test_opt(sbi, POSIX_ACL) ? MS_POSIXACL : 0);
- memcpy(sb->s_uuid, raw_super->uuid, sizeof(raw_super->uuid));
+ memcpy(&sb->s_uuid, raw_super->uuid, sizeof(raw_super->uuid));
/* init f2fs-specific super block info */
sbi->valid_super_block = valid_super_block;
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index ed67548b286c..b92135c202c2 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -203,7 +203,7 @@ static void gfs2_sb_in(struct gfs2_sbd *sdp, const void *buf)
memcpy(sb->sb_lockproto, str->sb_lockproto, GFS2_LOCKNAME_LEN);
memcpy(sb->sb_locktable, str->sb_locktable, GFS2_LOCKNAME_LEN);
- memcpy(s->s_uuid, str->sb_uuid, 16);
+ memcpy(&s->s_uuid, str->sb_uuid, 16);
}
/**
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index 7a515345610c..e77bc52b468f 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -71,25 +71,14 @@ static ssize_t fsname_show(struct gfs2_sbd *sdp, char *buf)
return snprintf(buf, PAGE_SIZE, "%s\n", sdp->sd_fsname);
}
-static int gfs2_uuid_valid(const u8 *uuid)
-{
- int i;
-
- for (i = 0; i < 16; i++) {
- if (uuid[i])
- return 1;
- }
- return 0;
-}
-
static ssize_t uuid_show(struct gfs2_sbd *sdp, char *buf)
{
struct super_block *s = sdp->sd_vfs;
- const u8 *uuid = s->s_uuid;
+
buf[0] = '\0';
- if (!gfs2_uuid_valid(uuid))
+ if (uuid_is_null(&s->s_uuid))
return 0;
- return snprintf(buf, PAGE_SIZE, "%pUB\n", uuid);
+ return snprintf(buf, PAGE_SIZE, "%pUB\n", &s->s_uuid);
}
static ssize_t freeze_show(struct gfs2_sbd *sdp, char *buf)
@@ -712,14 +701,13 @@ static int gfs2_uevent(struct kset *kset, struct kobject *kobj,
{
struct gfs2_sbd *sdp = container_of(kobj, struct gfs2_sbd, sd_kobj);
struct super_block *s = sdp->sd_vfs;
- const u8 *uuid = s->s_uuid;
add_uevent_var(env, "LOCKTABLE=%s", sdp->sd_table_name);
add_uevent_var(env, "LOCKPROTO=%s", sdp->sd_proto_name);
if (!test_bit(SDF_NOJOURNALID, &sdp->sd_flags))
add_uevent_var(env, "JOURNALID=%d", sdp->sd_lockstruct.ls_jid);
- if (gfs2_uuid_valid(uuid))
- add_uevent_var(env, "UUID=%pUB", uuid);
+ if (!uuid_is_null(&s->s_uuid))
+ add_uevent_var(env, "UUID=%pUB", &s->s_uuid);
return 0;
}
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index ca1646fbcaef..83005f486451 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -2062,7 +2062,7 @@ static int ocfs2_initialize_super(struct super_block *sb,
cbits = le32_to_cpu(di->id2.i_super.s_clustersize_bits);
bbits = le32_to_cpu(di->id2.i_super.s_blocksize_bits);
sb->s_maxbytes = ocfs2_max_file_offset(bbits, cbits);
- memcpy(sb->s_uuid, di->id2.i_super.s_uuid,
+ memcpy(&sb->s_uuid, di->id2.i_super.s_uuid,
sizeof(di->id2.i_super.s_uuid));
osb->osb_dx_mask = (1 << (cbits - bbits)) - 1;
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 682b336a7a6a..93a420160964 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -75,7 +75,7 @@ xfs_uuid_mount(
/* Publish UUID in struct super_block */
BUILD_BUG_ON(sizeof(mp->m_super->s_uuid) != sizeof(uuid_t));
- memcpy(&mp->m_super->s_uuid, uuid, sizeof(uuid_t));
+ uuid_copy(&mp->m_super->s_uuid, uuid);
if (mp->m_flags & XFS_MOUNT_NOUUID)
return 0;
diff --git a/include/linux/cleancache.h b/include/linux/cleancache.h
index fccf7f44139d..bbb3712dd892 100644
--- a/include/linux/cleancache.h
+++ b/include/linux/cleancache.h
@@ -27,7 +27,7 @@ struct cleancache_filekey {
struct cleancache_ops {
int (*init_fs)(size_t);
- int (*init_shared_fs)(char *uuid, size_t);
+ int (*init_shared_fs)(uuid_t *uuid, size_t);
int (*get_page)(int, struct cleancache_filekey,
pgoff_t, struct page *);
void (*put_page)(int, struct cleancache_filekey,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 26488b419965..cdffd2f86aa0 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -30,6 +30,7 @@
#include <linux/percpu-rwsem.h>
#include <linux/workqueue.h>
#include <linux/delayed_call.h>
+#include <linux/uuid.h>
#include <asm/byteorder.h>
#include <uapi/linux/fs.h>
@@ -1326,8 +1327,8 @@ struct super_block {
struct sb_writers s_writers;
- char s_id[32]; /* Informational name */
- u8 s_uuid[16]; /* UUID */
+ char s_id[32]; /* Informational name */
+ uuid_t s_uuid; /* UUID */
void *s_fs_info; /* Filesystem private info */
unsigned int s_max_links;
diff --git a/mm/cleancache.c b/mm/cleancache.c
index ba5d8f3e6d68..f7b9fdc79d97 100644
--- a/mm/cleancache.c
+++ b/mm/cleancache.c
@@ -130,7 +130,7 @@ void __cleancache_init_shared_fs(struct super_block *sb)
int pool_id = CLEANCACHE_NO_BACKEND_SHARED;
if (cleancache_ops) {
- pool_id = cleancache_ops->init_shared_fs(sb->s_uuid, PAGE_SIZE);
+ pool_id = cleancache_ops->init_shared_fs(&sb->s_uuid, PAGE_SIZE);
if (pool_id < 0)
pool_id = CLEANCACHE_NO_POOL;
}
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index d7f282d75cc1..1d32cd20009a 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -164,7 +164,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
hmac_misc.mode = inode->i_mode;
crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
if (evm_hmac_attrs & EVM_ATTR_FSUUID)
- crypto_shash_update(desc, inode->i_sb->s_uuid,
+ crypto_shash_update(desc, &inode->i_sb->s_uuid.b[0],
sizeof(inode->i_sb->s_uuid));
crypto_shash_final(desc, digest);
}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index da3e7d50e0d7..659dbcc83d2f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -244,7 +244,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
&& rule->fsmagic != inode->i_sb->s_magic)
return false;
if ((rule->flags & IMA_FSUUID) &&
- memcmp(&rule->fsuuid, inode->i_sb->s_uuid, sizeof(rule->fsuuid)))
+ !uuid_equal(&rule->fsuuid, &inode->i_sb->s_uuid))
return false;
if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
return false;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 14/16] block: remove blk_part_pack_uuid
From: Christoph Hellwig @ 2017-05-10 18:09 UTC (permalink / raw)
To: Andy Shevchenko, Amir Goldstein,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Shaohua Li,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, David Howells, Mimi Zohar,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-15-hch-jcswGhMUV9g@public.gmane.org>
On Wed, May 10, 2017 at 08:02:12PM +0200, Christoph Hellwig wrote:
> This helper was only used by IMA of all things, which would get spurious
> errors if CONFIG_BLOCK is disabled. Just opencode the call there.
>
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> [andy: correctly handle error returns from uuid_to_bin]
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> security/integrity/ima/ima_policy.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 4d1178610145..49fbc3e8f012 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -717,8 +717,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> break;
> }
>
> - uuid_to_bin(args[0].from, (uuid_t *)entry->fsuuid);
> - entry->flags |= IMA_FSUUID;
> + result = uuid_to_bin(args[0].from, (uuid_t *)&entry->fsuuid);
> + if (!result)
> + entry->flags |= IMA_FSUUID;
This should have been folded into the previous patch..
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Chris Murphy @ 2017-05-10 18:09 UTC (permalink / raw)
To: Edward Kuns; +Cc: Chris Murphy, Wols Lists, Linux-RAID
In-Reply-To: <CACsGCyQGoR_9dr7aYGZn1i2VWm8KWKgi1fy2x6met=UePw2OQw@mail.gmail.com>
On Wed, May 10, 2017 at 10:44 AM, Edward Kuns <eddie.kuns@gmail.com> wrote:
> On Tue, May 9, 2017 at 10:53 PM, Chris Murphy <lists@colorremedies.com> wrote:
>> Scrubs don't happen by default.
>
> From the perspective of Linux Raid authors, that is true. However,
> the version of Fedora I have installed on my server at home does
> weekly scrubs by default.
That is a check scrub, not a repair scrub, so it still wouldn't
obliterate "good" P & Q by default.
> This is arguably a good thing, considering
> that many people installing this OS will not proactively research the
> technologies in use holding their server together and won't know that
> there are certain maintenance activities that are essential if you
> care about your data.
>
> I'm not getting involved in the bigger discussion. My opinion is too
> uninformed to say anything there. I just wanted to point out that
> *from the viewpoint of some users*, scrubs *will* happen by default.
> That is all.
Absolutely, just not the kind of scrub that's being accused of
damaging assumed to be good P & Q parity.
--
Chris Murphy
^ permalink raw reply
* Re: [PATCH 03/16] uuid: rename uuid types
From: David Howells @ 2017-05-10 18:20 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Mimi Zohar, Amir Goldstein,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Andy Shevchenko, Shaohua Li,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-4-hch-jcswGhMUV9g@public.gmane.org>
Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> -#define NULL_UUID_LE \
> - UUID_LE(0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, \
> - 0x00, 0x00, 0x00, 0x00)
> +#define NULL_GUID \
> + GUID(0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, \
> + 0x00, 0x00, 0x00, 0x00)
>
> -#define NULL_UUID_BE \
> - UUID_BE(0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, \
> - 0x00, 0x00, 0x00, 0x00)
> +#define NULL_UUID \
> + UUID(0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, \
> + 0x00, 0x00, 0x00, 0x00)
These are UAPI and ought not to be renamed.
David
^ permalink raw reply
* Re: [PATCH 05/16] uuid: add the v1 layout to uuid_t
From: David Howells @ 2017-05-10 18:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Mimi Zohar, Amir Goldstein,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Andy Shevchenko, Shaohua Li,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-6-hch-jcswGhMUV9g@public.gmane.org>
Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> Turn the content of uuid_t into a union and add the fields for the v1
> interpretation to it.
>
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Reviewed-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply
* Re: [PATCH 06/16] afs: switch to use uuid_t and uuid_gen
From: David Howells @ 2017-05-10 18:22 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Mimi Zohar, Amir Goldstein,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dhowells-H+wXaHxf7aLQT0dZR+AlfA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Andy Shevchenko, Shaohua Li,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-7-hch-jcswGhMUV9g@public.gmane.org>
Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Reviewed-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply
* Re: Fault tolerance with badblocks
From: Nix @ 2017-05-10 19:03 UTC (permalink / raw)
To: NeilBrown; +Cc: Anthony Youngman, Phil Turmel, Ravi (Tom) Hale, linux-raid
In-Reply-To: <87lgq5n2c0.fsf@notabene.neil.brown.name>
On 9 May 2017, NeilBrown outgrape:
> On Tue, May 09 2017, Nix wrote:
>> Neil decided not to do any repair work in this case on the grounds that
>> if the drive is misdirecting one write it might misdirect the repair as
>> well
>
> My justification was a bit broader than that.
I noticed your trailing comment on the blog post only after sending all
these emails out :( bah!
> If you get a consistency error on RAID6, there is not one model to
> explain it which is significantly more likely than any other model.
Yeah, I'm quite satisfied with "we don't have enough data to know if
repairing is safe" as reasoning: among other things it suggests that
mismatches are really rare, which is reassuring! This certainly suggests
that repairing should be, at the very least, off by default, and I'm not
terribly unhappy for it to not exist.
... but I do want to at least report the location of stripes that fail
checks, as in my earlier ugly patch. That's useful for any array with >1
partition or LVM LV on it. ("Oh, that mismatch is harmless, it's in
swap. That one is in small_but_crucial_lv, I'll restore it from backup,
without affecting the massive_messy_lv which had no mismatches and would
take weeks to restore.")
(As far as I'm concerned, if you don't *have* a backup of some fs, you
deserve what's coming to you! Good backups are easy and with md you can
even make them as resilient as the main RAID arrays. I'm interested in
maximizing availability here: having to take a big array with many LVs
down for ages for a restore because you don't know which bit is
corrupted just seems *wrong*.)
^ permalink raw reply
* Re: [PATCH 16/16] fs: switch ->s_uuid to uuid_t
From: Amir Goldstein @ 2017-05-10 19:08 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs, linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Mimi Zohar, linux-kernel,
David Howells, linux-fsdevel, Andy Shevchenko, Shaohua Li,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-17-hch-jcswGhMUV9g@public.gmane.org>
On Wed, May 10, 2017 at 9:02 PM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> For some file systems we still memcpy into it, but in various places this
> already allows us to use the proper uuid helpers. More to come..
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> ---
Series looks good!
My main concern is that filesystems with char uuid[16] are here to stay,
or maybe, will be converted slowly.
Considering this, perhaps we should have a helper/macro to copy from
char[16] to *uuid_t and maybe BUILD_BUG_ON the size comparison?
#define bytes_to_uuid(dst, src) ...
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 682b336a7a6a..93a420160964 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -75,7 +75,7 @@ xfs_uuid_mount(
>
> /* Publish UUID in struct super_block */
> BUILD_BUG_ON(sizeof(mp->m_super->s_uuid) != sizeof(uuid_t));
This BUILD_BUG is no longer needed.
> - memcpy(&mp->m_super->s_uuid, uuid, sizeof(uuid_t));
> + uuid_copy(&mp->m_super->s_uuid, uuid);
>
> if (mp->m_flags & XFS_MOUNT_NOUUID)
> return 0;
[...]
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index d7f282d75cc1..1d32cd20009a 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -164,7 +164,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
> hmac_misc.mode = inode->i_mode;
> crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
> if (evm_hmac_attrs & EVM_ATTR_FSUUID)
> - crypto_shash_update(desc, inode->i_sb->s_uuid,
> + crypto_shash_update(desc, &inode->i_sb->s_uuid.b[0],
I wonder if 'inode->i_sb->s_uuid.b' would have been better here
Cheers,
Amir.
^ permalink raw reply
* Re: [PATCH 03/16] uuid: rename uuid types
From: Amir Goldstein @ 2017-05-10 19:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs, linux-raid-u79uwXL29TY76Z2rM5mHXA,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Mimi Zohar, linux-kernel,
David Howells, linux-fsdevel, Andy Shevchenko, Shaohua Li,
Steven Whitehouse
In-Reply-To: <20170510180214.16852-4-hch-jcswGhMUV9g@public.gmane.org>
On Wed, May 10, 2017 at 9:02 PM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> Our "little endian" UUID really is a Wintel GUID, so rename it and its
> helpers such (guid_t). The big endian UUID is the only true one, so
> give it the name uuid_t. The uuid_le and uuid_be names are retained for,
> but will hopefully go away soon.
>
> Also remove the existing typedef in XFS that's now been superceeded by
> the generic type name.
>
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> [andy: also updated the UUID_LE/UUID_BE macros including fallout]
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
[...]
>
> -static inline int uuid_le_cmp(const uuid_le u1, const uuid_le u2)
> +static inline int guid_cmp(const guid_t u1, const guid_t u2)
> {
> - return memcmp(&u1, &u2, sizeof(uuid_le));
> + return memcmp(&u1, &u2, sizeof(guid_t));
> }
>
> -static inline int uuid_be_cmp(const uuid_be u1, const uuid_be u2)
> +static inline int uuid_cmp(const uuid_t u1, const uuid_t u2)
> {
> - return memcmp(&u1, &u2, sizeof(uuid_be));
> + return memcmp(&u1, &u2, sizeof(uuid_t));
> }
>
I think we should use this opportunity to change these to (uuid_t *, uuid_t *)
which is more in line with the rest of the helpers including the new helpers
hoisted from xfs.
[...]
> +/* backwards compatibility, don't use in new code */
> +#define uuid_le_cmp(u1, u2) guid_cmp(u1, u2)
> +#define uuid_be_cmp(u1, u2) uuid_cmp(u1, u2)
Therefore:
#define uuid_le_cmp(u1, u2) guid_cmp(&(u1), &(u2))
#define uuid_be_cmp(u1, u2) uuid_cmp(&(u1), &(u2))
^ permalink raw reply
* Re: [PATCH v2] raid1: prefer disk without bad blocks
From: Shaohua Li @ 2017-05-10 19:57 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid
In-Reply-To: <1494422180-22772-1-git-send-email-tomasz.majchrzak@intel.com>
On Wed, May 10, 2017 at 03:16:20PM +0200, Tomasz Majchrzak wrote:
> If an array consists of two drives and the first drive has the bad
> block, the read request to the region overlapping the bad block chooses
> the same disk (with bad block) as device to read from over and over and
> the request gets stuck. If the first disk only partially overlaps with
> bad block, it becomes a candidate ("best disk") for shorter range of
> sectors. The second disk is capable of reading the entire requested
> range and it is updated accordingly, however it is not recorded as a
> best device for the request. In the end the request is sent to the first
> disk to read entire range of sectors. It fails and is re-tried in a
> moment but with the same outcome.
>
> Actually it is quite likely scenario but it had little exposure in my
> test until commit 715d40b93b10 ("md/raid1: add failfast handling for
> reads.") removed preference for idle disk. Such scenario had been passing
> as second disk was always chosen when idle.
>
> Update a candidate ("best disk") to read from if disk can read entire
> range. Do it only if other disk has already been chosen as a candidate
> for a smaller range. Otherwise, don't update it - let the head position /
> disk type logic to select a disk.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> ---
> drivers/md/raid1.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> v2:
> updated the title not to include internal defect number :)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index a1f3fbe..e7ab3bb 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -628,8 +628,11 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio, int *max_sect
> break;
> }
> continue;
> - } else
> + } else {
> + if ((sectors > best_good_sectors) && (best_disk >= 0))
> + best_disk = disk;
best_disk = -1 seems better here to me. If this is the only disk, we will
choose in below 'if (best_disk == -1)' check. If there are more disks which
cover the whole range, we will choose the best disk with minimum seek/pending.
Thanks,
Shaohua
> best_good_sectors = sectors;
> + }
>
> if (best_disk >= 0)
> /* At least two disks to choose from so failfast is OK */
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [RFC 09/10] ima: move to generic async completion
From: Mimi Zohar @ 2017-05-10 21:26 UTC (permalink / raw)
To: Gilad Ben-Yossef, Herbert Xu, David S. Miller, Jonathan Corbet,
David Howells, Alasdair Kergon, Mike Snitzer, dm-devel,
Shaohua Li, Steve French, Theodore Y. Ts'o, Jaegeuk Kim,
Dmitry Kasatkin, James Morris, Serge E. Hallyn
Cc: Ofir Drang, Gilad Ben-Yossef, linux-crypto, linux-doc,
linux-kernel, keyrings, linux-raid, linux-cifs, samba-technical,
linux-fsdevel, linux-ima-devel, linux-ima-user,
linux-security-module
In-Reply-To: <1494075602-5061-10-git-send-email-gilad@benyossef.com>
On Sat, 2017-05-06 at 15:59 +0300, Gilad Ben-Yossef wrote:
> ima starts several async. crypto ops and waits for their completions.
> Move it over to generic code doing the same.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
> security/integrity/ima/ima_crypto.c | 56 +++++++++++--------------------------
> 1 file changed, 17 insertions(+), 39 deletions(-)
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 802d5d2..0e4db1fe 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -27,11 +27,6 @@
>
> #include "ima.h"
>
> -struct ahash_completion {
> - struct completion completion;
> - int err;
> -};
> -
> /* minimum file size for ahash use */
> static unsigned long ima_ahash_minsize;
> module_param_named(ahash_minsize, ima_ahash_minsize, ulong, 0644);
> @@ -196,30 +191,13 @@ static void ima_free_atfm(struct crypto_ahash *tfm)
> crypto_free_ahash(tfm);
> }
>
> -static void ahash_complete(struct crypto_async_request *req, int err)
> +static inline int ahash_wait(int err, struct crypto_wait *wait)
> {
> - struct ahash_completion *res = req->data;
>
> - if (err == -EINPROGRESS)
> - return;
> - res->err = err;
> - complete(&res->completion);
> -}
> + err = crypto_wait_req(err, wait);
>
> -static int ahash_wait(int err, struct ahash_completion *res)
> -{
> - switch (err) {
> - case 0:
> - break;
> - case -EINPROGRESS:
> - case -EBUSY:
> - wait_for_completion(&res->completion);
> - reinit_completion(&res->completion);
> - err = res->err;
> - /* fall through */
> - default:
> + if (err)
> pr_crit_ratelimited("ahash calculation failed: err: %d\n", err);
> - }
>
> return err;
> }
> @@ -233,7 +211,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
> int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
> struct ahash_request *req;
> struct scatterlist sg[1];
> - struct ahash_completion res;
> + struct crypto_wait wait;
> size_t rbuf_size[2];
>
> hash->length = crypto_ahash_digestsize(tfm);
> @@ -242,12 +220,12 @@ static int ima_calc_file_hash_atfm(struct file *file,
> if (!req)
> return -ENOMEM;
>
> - init_completion(&res.completion);
> + crypto_init_wait(&wait);
> ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> CRYPTO_TFM_REQ_MAY_SLEEP,
> - ahash_complete, &res);
> + crypto_req_done, &wait);
>
> - rc = ahash_wait(crypto_ahash_init(req), &res);
> + rc = ahash_wait(crypto_ahash_init(req), &wait);
> if (rc)
> goto out1;
>
> @@ -288,7 +266,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
> * read/request, wait for the completion of the
> * previous ahash_update() request.
> */
> - rc = ahash_wait(ahash_rc, &res);
> + rc = ahash_wait(ahash_rc, &wait);
> if (rc)
> goto out3;
> }
> @@ -304,7 +282,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
> * read/request, wait for the completion of the
> * previous ahash_update() request.
> */
> - rc = ahash_wait(ahash_rc, &res);
> + rc = ahash_wait(ahash_rc, &wait);
> if (rc)
> goto out3;
> }
> @@ -318,7 +296,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
> active = !active; /* swap buffers, if we use two */
> }
> /* wait for the last update request to complete */
> - rc = ahash_wait(ahash_rc, &res);
> + rc = ahash_wait(ahash_rc, &wait);
> out3:
> if (read)
> file->f_mode &= ~FMODE_READ;
> @@ -327,7 +305,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
> out2:
> if (!rc) {
> ahash_request_set_crypt(req, NULL, hash->digest, 0);
> - rc = ahash_wait(crypto_ahash_final(req), &res);
> + rc = ahash_wait(crypto_ahash_final(req), &wait);
> }
> out1:
> ahash_request_free(req);
> @@ -527,7 +505,7 @@ static int calc_buffer_ahash_atfm(const void *buf, loff_t len,
> {
> struct ahash_request *req;
> struct scatterlist sg;
> - struct ahash_completion res;
> + struct crypto_wait wait;
> int rc, ahash_rc = 0;
>
> hash->length = crypto_ahash_digestsize(tfm);
> @@ -536,12 +514,12 @@ static int calc_buffer_ahash_atfm(const void *buf, loff_t len,
> if (!req)
> return -ENOMEM;
>
> - init_completion(&res.completion);
> + crypto_init_wait(&wait);
> ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
> CRYPTO_TFM_REQ_MAY_SLEEP,
> - ahash_complete, &res);
> + crypto_req_done, &wait);
>
> - rc = ahash_wait(crypto_ahash_init(req), &res);
> + rc = ahash_wait(crypto_ahash_init(req), &wait);
> if (rc)
> goto out;
>
> @@ -551,10 +529,10 @@ static int calc_buffer_ahash_atfm(const void *buf, loff_t len,
> ahash_rc = crypto_ahash_update(req);
>
> /* wait for the update request to complete */
> - rc = ahash_wait(ahash_rc, &res);
> + rc = ahash_wait(ahash_rc, &wait);
> if (!rc) {
> ahash_request_set_crypt(req, NULL, hash->digest, 0);
> - rc = ahash_wait(crypto_ahash_final(req), &res);
> + rc = ahash_wait(crypto_ahash_final(req), &wait);
> }
> out:
> ahash_request_free(req);
^ permalink raw reply
* Re: [PATCH v4 2/2] md/r5cache: gracefully handle journal device errors for writeback mode
From: Song Liu @ 2017-05-10 21:45 UTC (permalink / raw)
To: Shaohua Li
Cc: linux-raid@vger.kernel.org, Shaohua Li, neilb@suse.com,
Kernel Team, dan.j.williams@intel.com, hch@infradead.org,
jes.sorensen@gmail.com
In-Reply-To: <20170510170043.4v4ijoxmfty6hndf@kernel.org>
> On May 10, 2017, at 10:01 AM, Shaohua Li <shli@kernel.org> wrote:
>
> On Mon, May 08, 2017 at 05:39:25PM -0700, Song Liu wrote:
>> For the raid456 with writeback cache, when journal device failed during
>> normal operation, it is still possible to persist all data, as all
>> pending data is still in stripe cache. However, it is necessary to handle
>> journal failure gracefully.
>>
>> During journal failures, this patch makes the follow changes to land data
>> in cache to raid disks gracefully:
>>
>> 1. In handle_stripe(), allow stripes with data in journal (s.injournal > 0)
>> to make progress;
>> 2. In delay_towrite(), only process data in the cache (skip dev->towrite);
>> 3. In __get_priority_stripe(), set try_loprio to true, so no stripe stuck
>> in loprio_list
>
> Applied the first patch. For this patch, I don't have a clear picture about
> what you are trying to do. Please describe the steps we are doing to do after
> journal failure.
I will add more description to the next version.
>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> drivers/md/raid5-cache.c | 13 ++++++++++---
>> drivers/md/raid5-log.h | 3 ++-
>> drivers/md/raid5.c | 29 +++++++++++++++++++++++------
>> 3 files changed, 35 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
>> index dc1dba6..e6032f6 100644
>> --- a/drivers/md/raid5-cache.c
>> +++ b/drivers/md/raid5-cache.c
>> @@ -24,6 +24,7 @@
>> #include "md.h"
>> #include "raid5.h"
>> #include "bitmap.h"
>> +#include "raid5-log.h"
>>
>> /*
>> * metadata/data stored in disk with 4k size unit (a block) regardless
>> @@ -679,6 +680,7 @@ static void r5c_disable_writeback_async(struct work_struct *work)
>> return;
>> pr_info("md/raid:%s: Disabling writeback cache for degraded array.\n",
>> mdname(mddev));
>> + md_update_sb(mddev, 1);
>
> Why this? And md_update_sb must be called within mddev->reconfig_mutex locked.
This is to avoid skipping in handle_stripe():
if (s.handle_bad_blocks ||
test_bit(MD_SB_CHANGE_PENDING, &conf->mddev->sb_flags)) {
set_bit(STRIPE_HANDLE, &sh->state);
goto finish;
}
I haven't got a better idea than calling md_update_sb() somewhere. It is also tricky
to lock mddev->reconfigured_mutex here, due to potential deadlocking with
mddev->open_mutex.
Do you have suggestions on this?
>> mddev_suspend(mddev);
>> log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH;
>> mddev_resume(mddev);
>> @@ -1557,6 +1559,8 @@ void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
>> void r5l_quiesce(struct r5l_log *log, int state)
>> {
>> struct mddev *mddev;
>> + struct r5conf *conf;
>> +
>> if (!log || state == 2)
>> return;
>> if (state == 0)
>> @@ -1564,10 +1568,12 @@ void r5l_quiesce(struct r5l_log *log, int state)
>> else if (state == 1) {
>> /* make sure r5l_write_super_and_discard_space exits */
>> mddev = log->rdev->mddev;
>> + conf = mddev->private;
>> wake_up(&mddev->sb_wait);
>> kthread_park(log->reclaim_thread->tsk);
>> r5l_wake_reclaim(log, MaxSector);
>> - r5l_do_reclaim(log);
>> + if (!r5l_log_disk_error(conf))
>> + r5l_do_reclaim(log);
>
> I think r5c_disable_writeback_async() will call into this, so we flush all
> stripe cache out to raid disks, why skip the reclaim?
>
r5l_do_reclaim() reclaims log space with 2 steps:
1. clear all io_unit lists (flushing_ios, etc.) by waking up mddev->thread.
2. update log_tail in the journal device, and issue discard to journal device.
When we are handling log failures in r5c_disable_writeback_async(), we are
flushing the cache, so it is not necessary to wake up mddev->thread. Also,
with log device error, it is not necessary update log_tail or issue discard.
Therefore, r5l_do_reclaim is not necessary in log disk errors.
Thanks,
Song
^ permalink raw reply
* Re: RAID creation resync behaviors
From: Martin K. Petersen @ 2017-05-10 22:20 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Jes Sorensen, Martin K. Petersen, NeilBrown, Shaohua Li,
linux-raid, neilb
In-Reply-To: <f4485279-2146-5134-d292-d1566e465a38@suse.de>
Hannes,
>> It doesn't help me obtain the information I need to make a decision in
>> mdadm as whether to overwrite all or compare+write when resyncing a RAID
>> array.
>>
> What you actually want is the COMPARE AND WRITE SCSI command
Actually, I think what Jes needs is a MISCOMPARE AND WRITE command :)
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: Resync of the degraded RAID10 array
From: Shaohua Li @ 2017-05-10 22:27 UTC (permalink / raw)
To: Tomasz Majchrzak; +Cc: linux-raid
In-Reply-To: <20170510140044.GA23565@proton.igk.intel.com>
On Wed, May 10, 2017 at 04:00:44PM +0200, Tomasz Majchrzak wrote:
> Hi all,
>
> I wonder what should be the resync behaviour for the degraded RAID10 array.
>
> cat /proc/mdstat
> Personalities : [raid10]
> md127 : active raid10 nvme3n1[3] nvme2n1[2] nvme1n1[1] nvme0n1[0]
> 2097152 blocks super 1.2 512K chunks 2 near-copies [4/4] [UUUU]
> [==>..................] resync = 11.0% (232704/2097152) finish=0.1min speed=232704K/sec
>
> mdadm -If nvme3n1
> mdadm: set nvme3n1 faulty in md127
> mdadm: hot removed nvme3n1 from md127
>
> cat /proc/mdstat
> Personalities : [raid10]
> md127 : active (auto-read-only) raid10 nvme2n1[2] nvme1n1[1] nvme0n1[0]
> 2097152 blocks super 1.2 512K chunks 2 near-copies [4/3] [UUU_]
> resync=PENDING
>
> cat /sys/block/md127/md/resync_start
> 465408
>
> At the moment it stops the resync. When new disk is added to the array, the
Probably check why the resync is stopped. The resync can still continue in
degraded mode. After resync completes, the recovery will start.
Thanks,
Shaohua
> recovery starts and completes, however no resync for the first 2 disks takes
> place and array is reported as clean when it's really out-of-sync.
>
> My kernel version is 4.11.
>
> What is the expected behaviour? Shall resync continue on 3-disk RAID10 or
> shall it be restarted when recovery completes?
>
> Regards,
>
> Tomek
> --
> 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 v4 2/2] md/r5cache: gracefully handle journal device errors for writeback mode
From: Shaohua Li @ 2017-05-10 22:51 UTC (permalink / raw)
To: Song Liu
Cc: linux-raid@vger.kernel.org, Shaohua Li, neilb@suse.com,
Kernel Team, dan.j.williams@intel.com, hch@infradead.org,
jes.sorensen@gmail.com
In-Reply-To: <72F7A8AB-A6BD-4C52-B8AF-11C5E71CFEF4@fb.com>
On Wed, May 10, 2017 at 09:45:05PM +0000, Song Liu wrote:
>
> > On May 10, 2017, at 10:01 AM, Shaohua Li <shli@kernel.org> wrote:
> >
> > On Mon, May 08, 2017 at 05:39:25PM -0700, Song Liu wrote:
> >> For the raid456 with writeback cache, when journal device failed during
> >> normal operation, it is still possible to persist all data, as all
> >> pending data is still in stripe cache. However, it is necessary to handle
> >> journal failure gracefully.
> >>
> >> During journal failures, this patch makes the follow changes to land data
> >> in cache to raid disks gracefully:
> >>
> >> 1. In handle_stripe(), allow stripes with data in journal (s.injournal > 0)
> >> to make progress;
> >> 2. In delay_towrite(), only process data in the cache (skip dev->towrite);
> >> 3. In __get_priority_stripe(), set try_loprio to true, so no stripe stuck
> >> in loprio_list
> >
> > Applied the first patch. For this patch, I don't have a clear picture about
> > what you are trying to do. Please describe the steps we are doing to do after
> > journal failure.
>
> I will add more description to the next version.
>
> >
> >> Signed-off-by: Song Liu <songliubraving@fb.com>
> >> ---
> >> drivers/md/raid5-cache.c | 13 ++++++++++---
> >> drivers/md/raid5-log.h | 3 ++-
> >> drivers/md/raid5.c | 29 +++++++++++++++++++++++------
> >> 3 files changed, 35 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> >> index dc1dba6..e6032f6 100644
> >> --- a/drivers/md/raid5-cache.c
> >> +++ b/drivers/md/raid5-cache.c
> >> @@ -24,6 +24,7 @@
> >> #include "md.h"
> >> #include "raid5.h"
> >> #include "bitmap.h"
> >> +#include "raid5-log.h"
> >>
> >> /*
> >> * metadata/data stored in disk with 4k size unit (a block) regardless
> >> @@ -679,6 +680,7 @@ static void r5c_disable_writeback_async(struct work_struct *work)
> >> return;
> >> pr_info("md/raid:%s: Disabling writeback cache for degraded array.\n",
> >> mdname(mddev));
> >> + md_update_sb(mddev, 1);
> >
> > Why this? And md_update_sb must be called within mddev->reconfig_mutex locked.
>
> This is to avoid skipping in handle_stripe():
>
> if (s.handle_bad_blocks ||
> test_bit(MD_SB_CHANGE_PENDING, &conf->mddev->sb_flags)) {
> set_bit(STRIPE_HANDLE, &sh->state);
> goto finish;
> }
>
> I haven't got a better idea than calling md_update_sb() somewhere. It is also tricky
> to lock mddev->reconfigured_mutex here, due to potential deadlocking with
> mddev->open_mutex.
>
> Do you have suggestions on this?
This sounds not necessary then. The mddev->thread will call md_update_sb and
clear the MD_SB_CHANGE_PENDING bit. After that, the stripes will be handled.
This is running in a workqueue, I'm wondering what kind of deadlock issue there
is.
>
> >> mddev_suspend(mddev);
> >> log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH;
> >> mddev_resume(mddev);
> >> @@ -1557,6 +1559,8 @@ void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
> >> void r5l_quiesce(struct r5l_log *log, int state)
> >> {
> >> struct mddev *mddev;
> >> + struct r5conf *conf;
> >> +
> >> if (!log || state == 2)
> >> return;
> >> if (state == 0)
> >> @@ -1564,10 +1568,12 @@ void r5l_quiesce(struct r5l_log *log, int state)
> >> else if (state == 1) {
> >> /* make sure r5l_write_super_and_discard_space exits */
> >> mddev = log->rdev->mddev;
> >> + conf = mddev->private;
> >> wake_up(&mddev->sb_wait);
> >> kthread_park(log->reclaim_thread->tsk);
> >> r5l_wake_reclaim(log, MaxSector);
> >> - r5l_do_reclaim(log);
> >> + if (!r5l_log_disk_error(conf))
> >> + r5l_do_reclaim(log);
> >
> > I think r5c_disable_writeback_async() will call into this, so we flush all
> > stripe cache out to raid disks, why skip the reclaim?
> >
>
> r5l_do_reclaim() reclaims log space with 2 steps:
> 1. clear all io_unit lists (flushing_ios, etc.) by waking up mddev->thread.
> 2. update log_tail in the journal device, and issue discard to journal device.
>
> When we are handling log failures in r5c_disable_writeback_async(), we are
> flushing the cache, so it is not necessary to wake up mddev->thread. Also,
> with log device error, it is not necessary update log_tail or issue discard.
>
> Therefore, r5l_do_reclaim is not necessary in log disk errors.
Ok, there is no side effect, right? let's call it anyway.
Thanks,
Shaohua
^ permalink raw reply
* Re: [PATCH v4 2/2] md/r5cache: gracefully handle journal device errors for writeback mode
From: Song Liu @ 2017-05-10 23:58 UTC (permalink / raw)
To: Shaohua Li
Cc: linux-raid@vger.kernel.org, Shaohua Li, neilb@suse.com,
Kernel Team, dan.j.williams@intel.com, hch@infradead.org,
jes.sorensen@gmail.com
In-Reply-To: <20170510225103.gc34fpaqmesvadgi@kernel.org>
> On May 10, 2017, at 3:51 PM, Shaohua Li <shli@kernel.org> wrote:
>
> On Wed, May 10, 2017 at 09:45:05PM +0000, Song Liu wrote:
>>
>>> On May 10, 2017, at 10:01 AM, Shaohua Li <shli@kernel.org> wrote:
>>>
>>> On Mon, May 08, 2017 at 05:39:25PM -0700, Song Liu wrote:
>>>> For the raid456 with writeback cache, when journal device failed during
>>>> normal operation, it is still possible to persist all data, as all
>>>> pending data is still in stripe cache. However, it is necessary to handle
>>>> journal failure gracefully.
>>>>
>>>> During journal failures, this patch makes the follow changes to land data
>>>> in cache to raid disks gracefully:
>>>>
>>>> 1. In handle_stripe(), allow stripes with data in journal (s.injournal > 0)
>>>> to make progress;
>>>> 2. In delay_towrite(), only process data in the cache (skip dev->towrite);
>>>> 3. In __get_priority_stripe(), set try_loprio to true, so no stripe stuck
>>>> in loprio_list
>>>
>>> Applied the first patch. For this patch, I don't have a clear picture about
>>> what you are trying to do. Please describe the steps we are doing to do after
>>> journal failure.
>>
>> I will add more description to the next version.
>>
>>>
>>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>>> ---
>>>> drivers/md/raid5-cache.c | 13 ++++++++++---
>>>> drivers/md/raid5-log.h | 3 ++-
>>>> drivers/md/raid5.c | 29 +++++++++++++++++++++++------
>>>> 3 files changed, 35 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
>>>> index dc1dba6..e6032f6 100644
>>>> --- a/drivers/md/raid5-cache.c
>>>> +++ b/drivers/md/raid5-cache.c
>>>> @@ -24,6 +24,7 @@
>>>> #include "md.h"
>>>> #include "raid5.h"
>>>> #include "bitmap.h"
>>>> +#include "raid5-log.h"
>>>>
>>>> /*
>>>> * metadata/data stored in disk with 4k size unit (a block) regardless
>>>> @@ -679,6 +680,7 @@ static void r5c_disable_writeback_async(struct work_struct *work)
>>>> return;
>>>> pr_info("md/raid:%s: Disabling writeback cache for degraded array.\n",
>>>> mdname(mddev));
>>>> + md_update_sb(mddev, 1);
>>>
>>> Why this? And md_update_sb must be called within mddev->reconfig_mutex locked.
>>
>> This is to avoid skipping in handle_stripe():
>>
>> if (s.handle_bad_blocks ||
>> test_bit(MD_SB_CHANGE_PENDING, &conf->mddev->sb_flags)) {
>> set_bit(STRIPE_HANDLE, &sh->state);
>> goto finish;
>> }
>>
>> I haven't got a better idea than calling md_update_sb() somewhere. It is also tricky
>> to lock mddev->reconfigured_mutex here, due to potential deadlocking with
>> mddev->open_mutex.
>>
>> Do you have suggestions on this?
>
> This sounds not necessary then. The mddev->thread will call md_update_sb and
> clear the MD_SB_CHANGE_PENDING bit. After that, the stripes will be handled.
> This is running in a workqueue, I'm wondering what kind of deadlock issue there
> is.
mddev->thread calls md_check_recovery() in raid5d(), and md_check_recovery()
calls md_update_sb(). However, we need to call md_update_sb() before
mddev_suspend() here. Otherwise, mddev_suspend() will increase mddev->suspended
and then md_check_recovery() will exit before calling md_update_sb().
The deadlock of reconfig_mutex and open_mutex is as the following:
[ 211.753343] ======================================================
[ 211.753823] [ INFO: possible circular locking dependency detected ]
[ 211.754310] 4.11.0+ #88 Not tainted
[ 211.754582] -------------------------------------------------------
[ 211.755079] mdadm/1582 is trying to acquire lock:
[ 211.755444] ((&log->disable_writeback_work)){+.+...}, at: [<ffffffff8107d842>] flush_work+0x12/0x290
[ 211.756160]
[ 211.756160] but task is already holding lock:
[ 211.756609] (&mddev->reconfig_mutex){+.+.+.}, at: [<ffffffffa0052654>] rdev_attr_store+0x64/0xd0 [md_mod]
[ 211.757354]
[ 211.757354] which lock already depends on the new lock.
[ 211.757354]
[ 211.757984]
[ 211.757984] the existing dependency chain (in reverse order) is:
[ 211.758560]
[ 211.758560] -> #1 (&mddev->reconfig_mutex){+.+.+.}:
[ 211.759062] lock_acquire+0xc2/0x230
[ 211.759391] __mutex_lock+0x7b/0x9d0
[ 211.759716] mutex_lock_interruptible_nested+0x1b/0x20
[ 211.760158] r5c_disable_writeback_async+0x6b/0xa0 [raid456]
[ 211.760634] process_one_work+0x1d6/0x650
[ 211.760986] worker_thread+0x4d/0x380
[ 211.761311] kthread+0x117/0x150
[ 211.761602] ret_from_fork+0x2e/0x40
[ 211.761924]
[ 211.761924] -> #0 ((&log->disable_writeback_work)){+.+...}:
[ 211.762470] __lock_acquire+0x1458/0x1770
[ 211.762825] lock_acquire+0xc2/0x230
[ 211.763149] flush_work+0x4a/0x290
[ 211.763457] r5l_exit_log+0x31/0x80 [raid456]
[ 211.763840] raid5_remove_disk+0x8a/0x240 [raid456]
[ 211.764265] remove_and_add_spares+0x175/0x390 [md_mod]
[ 211.764714] state_store+0xa3/0x4f0 [md_mod]
[ 211.765090] rdev_attr_store+0x89/0xd0 [md_mod]
[ 211.765486] sysfs_kf_write+0x4f/0x70
[ 211.765815] kernfs_fop_write+0x147/0x1d0
[ 211.766170] __vfs_write+0x28/0x120
[ 211.766485] vfs_write+0xd3/0x1d0
[ 211.766789] SyS_write+0x52/0xa0
[ 211.767083] do_syscall_64+0x6a/0x210
[ 211.767408] return_from_SYSCALL_64+0x0/0x7a
[ 211.767782]
[ 211.767782] other info that might help us debug this:
[ 211.767782]
[ 211.768399] Possible unsafe locking scenario:
[ 211.768399]
[ 211.768861] CPU0 CPU1
[ 211.769214] ---- ----
[ 211.769567] lock(&mddev->reconfig_mutex);
[ 211.769897] lock((&log->disable_writeback_work));
[ 211.770471] lock(&mddev->reconfig_mutex);
[ 211.770993] lock((&log->disable_writeback_work));
>
>>
>>>> mddev_suspend(mddev);
>>>> log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH;
>>>> mddev_resume(mddev);
>>>> @@ -1557,6 +1559,8 @@ void r5l_wake_reclaim(struct r5l_log *log, sector_t space)
>>>> void r5l_quiesce(struct r5l_log *log, int state)
>>>> {
>>>> struct mddev *mddev;
>>>> + struct r5conf *conf;
>>>> +
>>>> if (!log || state == 2)
>>>> return;
>>>> if (state == 0)
>>>> @@ -1564,10 +1568,12 @@ void r5l_quiesce(struct r5l_log *log, int state)
>>>> else if (state == 1) {
>>>> /* make sure r5l_write_super_and_discard_space exits */
>>>> mddev = log->rdev->mddev;
>>>> + conf = mddev->private;
>>>> wake_up(&mddev->sb_wait);
>>>> kthread_park(log->reclaim_thread->tsk);
>>>> r5l_wake_reclaim(log, MaxSector);
>>>> - r5l_do_reclaim(log);
>>>> + if (!r5l_log_disk_error(conf))
>>>> + r5l_do_reclaim(log);
>>>
>>> I think r5c_disable_writeback_async() will call into this, so we flush all
>>> stripe cache out to raid disks, why skip the reclaim?
>>>
>>
>> r5l_do_reclaim() reclaims log space with 2 steps:
>> 1. clear all io_unit lists (flushing_ios, etc.) by waking up mddev->thread.
>> 2. update log_tail in the journal device, and issue discard to journal device.
>>
>> When we are handling log failures in r5c_disable_writeback_async(), we are
>> flushing the cache, so it is not necessary to wake up mddev->thread. Also,
>> with log device error, it is not necessary update log_tail or issue discard.
>>
>> Therefore, r5l_do_reclaim is not necessary in log disk errors.
>
> Ok, there is no side effect, right? let's call it anyway.
Let me double check potential side effect. If it is safe, I will remove it.
Thanks,
Song
^ permalink raw reply
* Re: [RFC 01/10] crypto: factor async completion for general use
From: Eric Biggers @ 2017-05-11 3:55 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Jonathan Corbet, David Howells,
Alasdair Kergon, Mike Snitzer, dm-devel, Shaohua Li, Steve French,
Theodore Y. Ts'o, Jaegeuk Kim, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, Ofir Drang, Gilad Ben-Yossef,
linux-crypto, linux-doc, linux-kernel, keyrings, linux-raid
In-Reply-To: <1494075602-5061-2-git-send-email-gilad@benyossef.com>
Hi Gilad,
On Sat, May 06, 2017 at 03:59:50PM +0300, Gilad Ben-Yossef wrote:
> Invoking a possibly async. crypto op and waiting for completion
> while correctly handling backlog processing is a common task
> in the crypto API implementation and outside users of it.
>
> This patch re-factors one of the in crypto API implementation in
> preparation for using it across the board instead of hand
> rolled versions.
Thanks for doing this! It annoyed me too that there wasn't a helper function
for this. Just a few comments below:
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 3556d8e..bf4acaf 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -480,33 +480,6 @@ int af_alg_cmsg_send(struct msghdr *msg, struct af_alg_control *con)
> }
> EXPORT_SYMBOL_GPL(af_alg_cmsg_send);
>
> -int af_alg_wait_for_completion(int err, struct af_alg_completion *completion)
> -{
> - switch (err) {
> - case -EINPROGRESS:
> - case -EBUSY:
> - wait_for_completion(&completion->completion);
> - reinit_completion(&completion->completion);
> - err = completion->err;
> - break;
> - };
> -
> - return err;
> -}
> -EXPORT_SYMBOL_GPL(af_alg_wait_for_completion);
> -
> -void af_alg_complete(struct crypto_async_request *req, int err)
> -{
> - struct af_alg_completion *completion = req->data;
> -
> - if (err == -EINPROGRESS)
> - return;
> -
> - completion->err = err;
> - complete(&completion->completion);
> -}
> -EXPORT_SYMBOL_GPL(af_alg_complete);
> -
I think it would be cleaner to switch af_alg and algif_* over to the new
interface in its own patch, rather than in the same patch that introduces the
new interface.
> @@ -88,8 +88,8 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg,
> if ((msg->msg_flags & MSG_MORE))
> hash_free_result(sk, ctx);
>
> - err = af_alg_wait_for_completion(crypto_ahash_init(&ctx->req),
> - &ctx->completion);
> + err = crypto_wait_req(crypto_ahash_init(&ctx->req),
> + &ctx->wait);
> if (err)
> goto unlock;
> }
In general can you try to keep the argument lists indented sanely? (This
applies throughout the patch series.) e.g. here I'd have expected:
err = crypto_wait_req(crypto_ahash_init(&ctx->req),
&ctx->wait);
>
> diff --git a/crypto/api.c b/crypto/api.c
> index 941cd4c..1c6e9cd 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -24,6 +24,7 @@
> #include <linux/sched/signal.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> +#include <linux/completion.h>
> #include "internal.h"
>
> LIST_HEAD(crypto_alg_list);
> @@ -595,5 +596,32 @@ int crypto_has_alg(const char *name, u32 type, u32 mask)
> }
> EXPORT_SYMBOL_GPL(crypto_has_alg);
>
> +int crypto_wait_req(int err, struct crypto_wait *wait)
> +{
> + switch (err) {
> + case -EINPROGRESS:
> + case -EBUSY:
> + wait_for_completion(&wait->completion);
> + reinit_completion(&wait->completion);
> + err = wait->err;
> + break;
> + };
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(crypto_wait_req);
crypto_wait_req() maybe should be inlined, since it doesn't do much (note that
reinit_completion is actually just a variable assignment), and the common case
is that 'err' will be 0, so there will be nothing to wait for.
Also drop the unnecessary semicolon at the end of the switch block.
With regards to the wait being uninterruptible, I agree that this should be the
default behavior, because I think users waiting for specific crypto requests are
generally not prepared to handle the wait actually being interrupted. After
interruption the crypto operation will still proceed in the background, and it
will use buffers which the caller has in many cases already freed. However, I'd
suggest taking a close look at anything that was actually doing an interruptible
wait before, to see whether it was a bug or intentional (or "doesn't matter").
And yes there could always be a crypto_wait_req_interruptible() introduced if
some users need it.
>
> #define CRYPTO_MINALIGN_ATTR __attribute__ ((__aligned__(CRYPTO_MINALIGN)))
>
> +/*
> + * Macro for declaring a crypto op async wait object on stack
> + */
> +#define DECLARE_CRYPTO_WAIT(_wait) \
> + struct crypto_wait _wait = { \
> + COMPLETION_INITIALIZER_ONSTACK((_wait).completion), 0 }
> +
Move this definition down below, so it's next to crypto_wait?
>
> /*
> * Algorithm registration interface.
> */
> @@ -1604,5 +1631,6 @@ static inline int crypto_comp_decompress(struct crypto_comp *tfm,
> src, slen, dst, dlen);
> }
>
> +
Don't add unrelated blank lines.
- Eric
^ permalink raw reply
* Re: [RFC 07/10] fscrypt: move to generic async completion
From: Eric Biggers @ 2017-05-11 4:04 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Jonathan Corbet, David Howells,
Alasdair Kergon, Mike Snitzer, dm-devel, Shaohua Li, Steve French,
Theodore Y. Ts'o, Jaegeuk Kim, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, Ofir Drang, Gilad Ben-Yossef,
linux-crypto, linux-doc, linux-kernel, keyrings, linux-raid
In-Reply-To: <1494075602-5061-8-git-send-email-gilad@benyossef.com>
Hi Gilad,
On Sat, May 06, 2017 at 03:59:56PM +0300, Gilad Ben-Yossef wrote:
> int fscrypt_do_page_crypto(const struct inode *inode, fscrypt_direction_t rw,
> u64 lblk_num, struct page *src_page,
> struct page *dest_page, unsigned int len,
> @@ -150,7 +135,7 @@ int fscrypt_do_page_crypto(const struct inode *inode, fscrypt_direction_t rw,
> u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)];
> } xts_tweak;
> struct skcipher_request *req = NULL;
> - DECLARE_FS_COMPLETION_RESULT(ecr);
> + DECLARE_CRYPTO_WAIT(ecr);
> struct scatterlist dst, src;
> struct fscrypt_info *ci = inode->i_crypt_info;
> struct crypto_skcipher *tfm = ci->ci_ctfm;
> @@ -168,7 +153,7 @@ int fscrypt_do_page_crypto(const struct inode *inode, fscrypt_direction_t rw,
[...]
This patch looks good --- thanks for doing this! I suggest also renaming 'ecr'
to 'wait' in the places being updated to use crypto_wait, since the name is
obsolete (and was already; it originally stood for "ext4 completion result").
- Eric
^ permalink raw reply
* Re: [PATCH 25/27] block: remove the discard_zeroes_data flag
From: Nicholas A. Bellinger @ 2017-05-11 4:50 UTC (permalink / raw)
To: hch-jcswGhMUV9g@public.gmane.org
Cc: axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
linux-raid-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org,
snitzer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
philipp.reisner-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
target-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Bart Van Assche,
lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
shli-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
agk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
drbd-dev-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org
In-Reply-To: <20170510140627.GA23759-jcswGhMUV9g@public.gmane.org>
On Wed, 2017-05-10 at 16:06 +0200, hch-jcswGhMUV9g@public.gmane.org wrote:
> On Mon, May 08, 2017 at 11:46:14PM -0700, Nicholas A. Bellinger wrote:
> > That said, simply propagating up q->limits.max_write_zeroes_sectors as
> > dev_attrib->unmap_zeroes_data following existing code still looks like
> > the right thing to do.
>
> It is not. Martin has decoupled write same/zeroes support from discard
> support. Any device will claim to support it initially, and we'll
> only clear the flag if a Write Same command fails.
>
> So even if LBPRZ is not set you can trivially get into a situation
> where discard is supported through UNMAP, and you'll incorrectly
> set LBPRZ and will cause data corruption.
In that case, there are two choices.
1) Expose a block_device or request_queue bit to signal 'real LBPRZ'
support up to IBLOCK, in order to maintain SCSI target feature
compatibility.
2) Or drop the LBPRZ bit usage for IBLOCK all-together.
Since I happen happen to support a block driver that has 'real LBPRZ'
support for all discards, I'd prefer the latter so this doesn't have to
be carried out-of-tree.
So what are the options for this in post v4.12..?
^ permalink raw reply
* Re: [PATCH 25/27] block: remove the discard_zeroes_data flag
From: hch-jcswGhMUV9g @ 2017-05-11 6:26 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
linux-raid-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org,
snitzer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
philipp.reisner-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
target-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Bart Van Assche,
lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
shli-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
hch-jcswGhMUV9g@public.gmane.org,
agk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
drbd-dev-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org
In-Reply-To: <1494478235.16894.115.camel-XoQW25Eq2zviZyQQd+hFbcojREIfoBdhmpATvIKMPHk@public.gmane.org>
On Wed, May 10, 2017 at 09:50:35PM -0700, Nicholas A. Bellinger wrote:
> 1) Expose a block_device or request_queue bit to signal 'real LBPRZ'
> support up to IBLOCK, in order to maintain SCSI target feature
> compatibility.
No way. If you want to zero use REQ_OP_WRITE_ZEROES..
^ permalink raw reply
* Re: [PATCH 25/27] block: remove the discard_zeroes_data flag
From: Nicholas A. Bellinger @ 2017-05-11 6:36 UTC (permalink / raw)
To: hch-jcswGhMUV9g@public.gmane.org
Cc: axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org,
linux-raid-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
martin.petersen-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org,
snitzer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
philipp.reisner-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
dm-devel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
target-devel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Bart Van Assche,
lars.ellenberg-63ez5xqkn6DQT0dZR+AlfA@public.gmane.org,
shli-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
agk-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org,
drbd-dev-cunTk1MwBs8qoQakbn7OcQ@public.gmane.org
In-Reply-To: <20170511062630.GA18517-jcswGhMUV9g@public.gmane.org>
On Thu, 2017-05-11 at 08:26 +0200, hch-jcswGhMUV9g@public.gmane.org wrote:
> On Wed, May 10, 2017 at 09:50:35PM -0700, Nicholas A. Bellinger wrote:
> > 1) Expose a block_device or request_queue bit to signal 'real LBPRZ'
> > support up to IBLOCK, in order to maintain SCSI target feature
> > compatibility.
>
> No way. If you want to zero use REQ_OP_WRITE_ZEROES..
Yes, I understand that part and it's what the earlier conversion of
IBLOCK to use blkdev_issue_zeroout() already does.
Once the blkdev_issue_zeroout() conversion is in place then LBPRZ can
always be set to one for IBLOCK using blkdev_issue_zeroout().
The point is this is not in -rc1, which as-is breaks LBPRZ compat for a
release.
^ permalink raw reply
* Re: [RFC 01/10] crypto: factor async completion for general use
From: Gilad Ben-Yossef @ 2017-05-11 7:29 UTC (permalink / raw)
To: Eric Biggers
Cc: Herbert Xu, David S. Miller, Jonathan Corbet, David Howells,
Alasdair Kergon, Mike Snitzer, dm-devel, Shaohua Li, Steve French,
Theodore Y. Ts'o, Jaegeuk Kim, Mimi Zohar, Dmitry Kasatkin,
James Morris, Serge E. Hallyn, Ofir Drang, Gilad Ben-Yossef,
linux-crypto, linux-doc, Linux kernel mailing list, keyrings
In-Reply-To: <20170511035527.GA2936@zzz>
Hi Eric,
On Thu, May 11, 2017 at 6:55 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> Hi Gilad,
>
> On Sat, May 06, 2017 at 03:59:50PM +0300, Gilad Ben-Yossef wrote:
>> Invoking a possibly async. crypto op and waiting for completion
>> while correctly handling backlog processing is a common task
>> in the crypto API implementation and outside users of it.
>>
>> This patch re-factors one of the in crypto API implementation in
>> preparation for using it across the board instead of hand
>> rolled versions.
>
> Thanks for doing this! It annoyed me too that there wasn't a helper function
> for this. Just a few comments below:
>
Thank you for the review.
I will incorporate the feedback into v2.
...
> With regards to the wait being uninterruptible, I agree that this should be the
> default behavior, because I think users waiting for specific crypto requests are
> generally not prepared to handle the wait actually being interrupted. After
> interruption the crypto operation will still proceed in the background, and it
> will use buffers which the caller has in many cases already freed. However, I'd
> suggest taking a close look at anything that was actually doing an interruptible
> wait before, to see whether it was a bug or intentional (or "doesn't matter").
>
> And yes there could always be a crypto_wait_req_interruptible() introduced if
> some users need it.
So this one was a bit of a shocker. I though the _interruptible use
sites seemed
wrong in the sense of being needless. However, after reading your feedback and
reviewing the code I'm pretty sure every single one of them (including
the one I've
added in dm-verity-target.c this merge window) are down right dangerous and
can cause random data corruption... so thanks for pointing this out!
I though of this patch set as a "make the code pretty" for 4.13 kind
of patch set.
Looks like it's a bug fix now, maybe even stable material.
Anyway, I'll roll a v2 and we'll see.
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
^ 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