Re: [dm-devel] [PATCH 5/6] libmultipath: simplify failed wwid code
On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote: > The (is|mark|unmark)_failed_wwid code is needlessly complicated. > Locking a file is necssary if multiple processes could otherwise be > writing to it at the same time. That is not the case with the > failed_wwids files. They can simply be empty files in a > directory. Even > with all the locking in place, two processes accessing or modifying a > file at the same time will still race. And even without the locking, > if > two processes try to access or modify a file at the same time, they > will > both see a reasonable result, and will leave the files in a valid > state > afterwards. > > Instead of doing all the locking work (which made it necessary to > write > a file, even just to check if a file existed), simply check for files > with lstat(), create them with open(), and remove them with unlink(). > > Signed-off-by: Benjamin Marzinski With the follow-up (6/6): Reviewed-by: Martin Wilck -- Dr. Martin Wilck , Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 4/6] Unit tests for is_path_valid()
On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski
Two minor nits below, otherwise ack.
> ---
> tests/Makefile | 4 +-
> tests/valid.c | 424
> +
> 2 files changed, 427 insertions(+), 1 deletion(-)
> create mode 100644 tests/valid.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 1b8706a7..125553b8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -13,7 +13,7 @@ CFLAGS += $(BIN_CFLAGS) -I$(multipathdir)
> -I$(mpathcmddir) \
> LIBDEPS += -L$(multipathdir) -L$(mpathcmddir) -lmultipath -lmpathcmd
> -lcmocka
>
> TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd
> pgpolicy \
> - alias directio
> + alias directio valid
>
> .SILENT: $(TESTS:%=%.o)
> .PRECIOUS: $(TESTS:%=%-test)
> @@ -50,6 +50,8 @@ vpd-test_OBJDEPS := ../libmultipath/discovery.o
> vpd-test_LIBDEPS := -ludev -lpthread -ldl
> alias-test_TESTDEPS := test-log.o
> alias-test_LIBDEPS := -lpthread -ldl
> +valid-test_OBJDEPS := ../libmultipath/valid.o
> +valid-test_LIBDEPS := -ludev -lpthread -ldl
> ifneq ($(DIO_TEST_DEV),)
> directio-test_LIBDEPS := -laio
> endif
> diff --git a/tests/valid.c b/tests/valid.c
> new file mode 100644
> index ..b128b029
> --- /dev/null
> +++ b/tests/valid.c
> @@ -0,0 +1,424 @@
> +/*
> + * Copyright (c) 2020 Benjamin Marzinski, Redhat
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <
> https://www.gnu.org/licenses/>;.
> + *
> + */
> +
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "globals.c"
> +#include "util.h"
> +#include "discovery.h"
> +#include "wwids.h"
> +#include "valid.h"
> +
> +int test_fd;
> +struct udev_device {
> + int unused;
> +} test_udev;
> +
> +bool __wrap_sysfs_is_multipathed(struct path *pp, bool set_wwid)
> +{
> + bool is_multipathed = mock_type(bool);
> + assert_non_null(pp);
> + assert_int_not_equal(strlen(pp->dev), 0);
> + if (is_multipathed && set_wwid)
> + strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
> + return is_multipathed;
> +}
> +
> +int __wrap___mpath_connect(int nonblocking)
> +{
> + bool connected = mock_type(bool);
> + assert_int_equal(nonblocking, 1);
> + if (connected)
> + return test_fd;
> + errno = mock_type(int);
> + return -1;
> +}
> +
> +int __wrap_systemd_service_enabled(const char *dev)
> +{
> + return (int)mock_type(bool);
> +}
> +
> +/* There's no point in checking the return value here */
> +int __wrap_mpath_disconnect(int fd)
> +{
> + assert_int_equal(fd, test_fd);
> + return 0;
> +}
> +
> +struct udev_device
> *__wrap_udev_device_new_from_subsystem_sysname(struct udev *udev,
> const char *subsystem, const char *sysname)
> +{
> + bool passed = mock_type(bool);
> + assert_string_equal(sysname, mock_ptr_type(char *));
> + if (passed)
> + return _udev;
> + return NULL;
> +}
> +
> +int __wrap_pathinfo(struct path *pp, struct config *conf, int mask)
> +{
> + int ret = mock_type(int);
> + assert_string_equal(pp->dev, mock_ptr_type(char *));
> + assert_int_equal(mask, DI_SYSFS | DI_WWID | DI_BLACKLIST);
> + if (ret == PATHINFO_OK)
> + strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
> + else
> + memset(pp->wwid, 0, WWID_SIZE);
> + return ret;
> +}
> +
> +int __wrap_is_failed_wwid(const char *wwid)
> +{
> + int ret = mock_type(int);
> + assert_string_equal(wwid, mock_ptr_type(char *));
> + return ret;
> +}
> +
> +int __wrap_check_wwids_file(char *wwid, int write_wwid)
> +{
> + bool passed = mock_type(bool);
> + assert_int_equal(write_wwid, 0);
> + assert_string_equal(wwid, mock_ptr_type(char *));
> + if (passed)
> + return 0;
> + else
> + return -1;
> +}
> +
> +int __wrap_dm_map_present_by_uuid(const char *uuid)
> +{
> + int ret = mock_type(int);
> + assert_string_equal(uuid, mock_ptr_type(char *));
> + return ret;
> +}
> +
> +enum {
> + STAGE_IS_MULTIPATHED,
> + STAGE_CHECK_MULTIPATHD,
> + STAGE_GET_UDEV_DEVICE,
> + STAGE_PATHINFO,
> + STAGE_IS_FAILED,
> + STAGE_CHECK_WWIDS,
> + STAGE_UUID_PRESENT,
> +};
nice :-)
> +
> +/* setup the test to continue past the given
Re: [dm-devel] [PATCH 3/6] multipath: centralize validation code
On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote:
> This code pulls the multipath path validation code out of
> configure(),
> and puts it into its own function, check_path_valid(). This function
> calls a new libmultipath function, is_path_valid() to check just path
> requested. This seperation exists so that is_path_valid() can be
> reused
> by future code. This code will give almost the same answer as the
> existing code, with the exception that now, if a device is currently
> multipathed, it will always be a valid multipath path.
>
> Signed-off-by: Benjamin Marzinski
Great job getting the logic right! Readability massively improved.
Almost ack, a few comments and questions below.
Regards,
Martin
> ---
> libmultipath/Makefile| 2 +-
> libmultipath/devmapper.c | 49 +++
> libmultipath/devmapper.h | 1 +
> libmultipath/structs.h | 24 +---
> libmultipath/valid.c | 118
> libmultipath/valid.h | 32 +
> libmultipath/wwids.c | 10 +-
> multipath/main.c | 296 +--
>
> 8 files changed, 337 insertions(+), 195 deletions(-)
> create mode 100644 libmultipath/valid.c
> create mode 100644 libmultipath/valid.h
>
> diff --git a/libmultipath/Makefile b/libmultipath/Makefile
> index f19b7ad2..e5dac5ea 100644
> --- a/libmultipath/Makefile
> +++ b/libmultipath/Makefile
> @@ -48,7 +48,7 @@ OBJS = memory.o parser.o vector.o devmapper.o
> callout.o \
> log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \
> lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \
> io_err_stat.o dm-generic.o generic.o foreign.o nvme-lib.o \
> - libsg.o
> + libsg.o valid.o
>
> all: $(LIBS)
>
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 7ed494a1..92f61133 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -770,6 +770,55 @@ out:
> return r;
> }
>
> +/*
> + * Return
> + * 1 : map with uuid exists
> + * 0 : map with uuid doesn't exist
> + * -1 : error
> + */
> +int
> +dm_map_present_by_uuid(const char *uuid)
> +{
> + struct dm_task *dmt;
> + struct dm_info info;
> + char prefixed_uuid[WWID_SIZE + UUID_PREFIX_LEN];
> + int r = -1;
> +
> + if (!uuid || uuid[0] == '\0')
> + return 0;
> +
> + if (safe_sprintf(prefixed_uuid, UUID_PREFIX "%s", uuid))
> + goto out;
> +
> + if (!(dmt = dm_task_create(DM_DEVICE_INFO)))
> + goto out;
> +
> + dm_task_no_open_count(dmt);
> +
> + if (!dm_task_set_uuid(dmt, prefixed_uuid))
> + goto out_task;
> +
> + if (!dm_task_run(dmt))
> + goto out_task;
> +
> + if (!dm_task_get_info(dmt, ))
> + goto out_task;
> +
> + r = 0;
> +
> + if (!info.exists)
> + goto out_task;
> +
> + r = 1;
I have nothing against goto for error handling, but here I'd prefer
r = !!info.exists;
> +out_task:
> + dm_task_destroy(dmt);
> +out:
> + if (r < 0)
> + condlog(3, "%s: dm command failed in %s: %s", uuid,
> + __FUNCTION__, strerror(errno));
> + return r;
> +}
> +
> static int
> dm_dev_t (const char * mapname, char * dev_t, int len)
> {
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index 17fc9faf..5ed7edc5 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -39,6 +39,7 @@ int dm_simplecmd_noflush (int, const char *,
> uint16_t);
> int dm_addmap_create (struct multipath *mpp, char *params);
> int dm_addmap_reload (struct multipath *mpp, char *params, int
> flush);
> int dm_map_present (const char *);
> +int dm_map_present_by_uuid(const char *uuid);
> int dm_get_map(const char *, unsigned long long *, char *);
> int dm_get_status(const char *, char *);
> int dm_type(const char *, char *);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 9bd39eb1..d69bc2e9 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -101,29 +101,13 @@ enum yes_no_undef_states {
> YNU_YES,
> };
>
> -#define _FIND_MULTIPATHS_F (1 << 1)
> -#define _FIND_MULTIPATHS_I (1 << 2)
> -#define _FIND_MULTIPATHS_N (1 << 3)
> -/*
> - * _FIND_MULTIPATHS_F must have the same value as YNU_YES.
> - * Generate a compile time error if that isn't the case.
> - */
> -extern char ___error1___[-(_FIND_MULTIPATHS_F != YNU_YES)];
> -
> -#define find_multipaths_on(conf) \
> - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_F))
> -#define ignore_wwids_on(conf) \
> - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_I))
> -#define ignore_new_devs_on(conf) \
> - (!!((conf)->find_multipaths & _FIND_MULTIPATHS_N))
> -
> enum find_multipaths_states {
> FIND_MULTIPATHS_UNDEF = YNU_UNDEF,
> FIND_MULTIPATHS_OFF = YNU_NO,
> - FIND_MULTIPATHS_ON = _FIND_MULTIPATHS_F,
> - FIND_MULTIPATHS_GREEDY = _FIND_MULTIPATHS_I,
> - FIND_MULTIPATHS_SMART =
Re: [dm-devel] [PATCH 2/6] libmultipath: make sysfs_is_multipathed able to return wwid
On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote: > sysfs_is_multipathed reads the wwid of the dm device holding a path > to > check if its a multipath device. Add code to optinally set pp->wwid > to > that wwid. This will be used by a future patch. > > Signed-off-by: Benjamin Marzinski Reviewed-by: Martin Wilck -- Dr. Martin Wilck , Tel. +49 (0)911 74053 2107 SUSE Software Solutions Germany GmbH HRB 36809, AG Nürnberg GF: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/6] multipath: path validation library prep work
On Thu, May 14, 2020 at 08:59:16PM -0500, Benjamin Marzinski wrote: > I've been playing around with the SID code more and I've decided to hold > off on submitting the library until I have it working with the SID > multipath module better. Instead, I've pulled out the common code that > multipath -u/-c and the library can use, and put it into libmultipath. > > I've also removed some of the ordering differences between the existing > code and my new code. Right now, the only difference is that if a path > is currently multipathed, it will always be claimed as a valid path. > > Patches 0001 & 0002 are the same as in my "RFC PATCH v2" set, and patch > 0005 is the same as my "libmultipath: simplify failed wwid code" patch. > > Only patches 0003 and 0004 haven't been posted before. > Just a note on applying this. This patch set is meant to be applied on top of Martin's recent patchsets. -Ben > Benjamin Marzinski (5): > libmultipath: make libmp_dm_init optional > libmultipath: make sysfs_is_multipathed able to return wwid > multipath: centralize validation code > Unit tests for is_path_valid() > libmultipath: simplify failed wwid code > > Martin Wilck (1): > libmultipath: use atomic linkat() in mark_failed_wwid() > > libmultipath/Makefile| 2 +- > libmultipath/devmapper.c | 66 +- > libmultipath/devmapper.h | 4 +- > libmultipath/structs.h | 24 +-- > libmultipath/sysfs.c | 24 ++- > libmultipath/sysfs.h | 2 +- > libmultipath/valid.c | 118 +++ > libmultipath/valid.h | 32 +++ > libmultipath/wwids.c | 165 --- > multipath/main.c | 295 --- > tests/Makefile | 4 +- > tests/valid.c| 424 +++ > 12 files changed, 875 insertions(+), 285 deletions(-) > create mode 100644 libmultipath/valid.c > create mode 100644 libmultipath/valid.h > create mode 100644 tests/valid.c > > -- > 2.17.2 > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [dm:for-next 53/58] drivers/md/dm-zoned-metadata.c:779:3: error: implicit declaration of function 'export_uuid'
On Fri, May 15 2020 at 3:25am -0400,
Hannes Reinecke wrote:
> Hi Mike,
>
> This is cause by missing commit d01cd62400b3 ("uuid: Add inline
> helpers to import / export UUIDs"), which went into 5.7.
> Mind to update your tree?
Rebased on v5.7-rc5, thanks
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [dm:for-next 53/58] drivers/md/dm-zoned-metadata.c:779:3: error: implicit declaration of function 'export_uuid'
Hi Mike,
On 5/15/20 8:32 AM, kbuild test robot wrote:
tree:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git
for-next
head: 6c871f63986b34c7768080259ddf5991c55ee385
commit: 70978208ec91d798066f4c291bc98ff914bea222 [53/58] dm zoned: metadata
version 2
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 70978208ec91d798066f4c291bc98ff914bea222
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross
ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/md/dm-zoned-metadata.c: In function 'dmz_write_sb':
drivers/md/dm-zoned-metadata.c:779:3: error: implicit declaration of function
'export_uuid' [-Werror=implicit-function-declaration]
779 | export_uuid(sb->dmz_uuid, >uuid);
| ^~~
drivers/md/dm-zoned-metadata.c: In function 'dmz_check_sb':
drivers/md/dm-zoned-metadata.c:1015:3: error: implicit declaration of function
'import_uuid' [-Werror=implicit-function-declaration]
1015 | import_uuid(_uuid, sb->dmz_uuid);
| ^~~
cc1: some warnings being treated as errors
vim +/export_uuid +779 drivers/md/dm-zoned-metadata.c
761
762 /*
763 * Write super block of the specified metadata set.
764 */
765 static int dmz_write_sb(struct dmz_metadata *zmd, unsigned int set)
766 {
767 struct dmz_mblock *mblk = zmd->sb[set].mblk;
768 struct dmz_super *sb = zmd->sb[set].sb;
769 struct dmz_dev *dev = zmd->sb[set].dev;
770 sector_t sb_block;
771 u64 sb_gen = zmd->sb_gen + 1;
772 int ret;
773
774 sb->magic = cpu_to_le32(DMZ_MAGIC);
775
776 sb->version = cpu_to_le32(zmd->sb_version);
777 if (zmd->sb_version > 1) {
778 BUILD_BUG_ON(UUID_SIZE != 16);
> 779 export_uuid(sb->dmz_uuid, >uuid);
780 memcpy(sb->dmz_label, zmd->label, BDEVNAME_SIZE);
781 export_uuid(sb->dev_uuid, >uuid);
782 }
783
784 sb->gen = cpu_to_le64(sb_gen);
785
786 /*
787 * The metadata always references the absolute block address,
788 * ie relative to the entire block range, not the per-device
789 * block address.
790 */
791 sb_block = zmd->sb[set].zone->id << zmd->zone_nr_blocks_shift;
792 sb->sb_block = cpu_to_le64(sb_block);
793 sb->nr_meta_blocks = cpu_to_le32(zmd->nr_meta_blocks);
794 sb->nr_reserved_seq = cpu_to_le32(zmd->nr_reserved_seq);
795 sb->nr_chunks = cpu_to_le32(zmd->nr_chunks);
796
797 sb->nr_map_blocks = cpu_to_le32(zmd->nr_map_blocks);
798 sb->nr_bitmap_blocks = cpu_to_le32(zmd->nr_bitmap_blocks);
799
800 sb->crc = 0;
801 sb->crc = cpu_to_le32(crc32_le(sb_gen, (unsigned char *)sb,
DMZ_BLOCK_SIZE));
802
803 ret = dmz_rdwr_block(dev, REQ_OP_WRITE, zmd->sb[set].block,
804 mblk->page);
805 if (ret == 0)
806 ret = blkdev_issue_flush(dev->bdev, GFP_NOIO, NULL);
807
808 return ret;
809 }
810
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
This is cause by missing commit d01cd62400b3 ("uuid: Add inline helpers
to import / export UUIDs"), which went into 5.7.
Mind to update your tree?
Cheers,
Hannes
--
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 07/10] block: use sectors_to_npage() and PAGE_SECTORS to clean up code
On 2020/5/15 12:19, Matthew Wilcox wrote:
> On Thu, May 07, 2020 at 03:50:57PM +0800, Zhen Lei wrote:
>> +++ b/block/blk-settings.c
>> @@ -150,7 +150,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q,
>> unsigned int max_hw_secto
>> unsigned int max_sectors;
>>
>> if ((max_hw_sectors << 9) < PAGE_SIZE) {
>> -max_hw_sectors = 1 << (PAGE_SHIFT - 9);
>> +max_hw_sectors = PAGE_SECTORS;
>
> Surely this should be:
>
> if (max_hw_sectors < PAGE_SECTORS) {
> max_hw_sectors = PAGE_SECTORS;
>
> ... no?
I've noticed this place before. "(max_hw_sectors << 9) < PAGE_SIZE" can also
make sure
that max_hw_sectors is not too large, that means (max_hw_sectors << 9) may
overflow.
>
>> -page = read_mapping_page(mapping,
>> -(pgoff_t)(n >> (PAGE_SHIFT - 9)), NULL);
>> +page = read_mapping_page(mapping, (pgoff_t)sectors_to_npage(n), NULL);
>
> ... again, get the type right, and you won't need the cast.
OK, I'll consider it.
>
>
> .
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [RFC PATCH v3 00/12] Integrity Policy Enforcement LSM (IPE)
On 12/05/2020 22:46, Deven Bowers wrote:
>
>
> On 5/11/2020 11:03 AM, Deven Bowers wrote:
>>
>>
>> On 5/10/2020 2:28 AM, Mickaël Salaün wrote:
>>
>> [...snip]
>>
Additionally, rules are evaluated top-to-bottom. As a result, any
revocation rules, or denies should be placed early in the file to
ensure
that these rules are evaluated before a rule with "action=ALLOW" is
hit.
IPE policy is designed to be forward compatible and backwards
compatible,
thus any failure to parse a rule will result in the line being ignored,
and a warning being emitted. If backwards compatibility is not
required,
the kernel commandline parameter and sysctl, ipe.strict_parse can be
enabled, which will cause these warnings to be fatal.
>>>
>>> Ignoring unknown command may lead to inconsistent beaviors. To achieve
>>> forward compatibility, I think it would be better to never ignore
>>> unknown rule but to give a way to userspace to known what is the current
>>> kernel ABI. This could be done with a securityfs file listing the
>>> current policy grammar.
>>>
>>
>> That's a fair point. From a manual perspective, I think this is fine.
>> A human-user can interpret a grammar successfully on their own when new
>> syntax is introduced.
>>
>> From a producing API perspective, I'd have to think about it a bit
>> more. Ideally, the grammar would be structured in such a way that the
>> userland
>> interpreter of this grammar would not have to be updated once new syntax
>> is introduced, avoiding the need to update the userland binary. To do so
>> generically ("op=%s") is easy, but doesn't necessarily convey sufficient
>> information (what happens when a new "op" token is introduced?). I think
>> this may come down to regular expression representations of valid values
>> for these tokens, which worries me as regular expressions are incredibly
>> error-prone[1].
>>
>> I'll see what I can come up with regarding this.
>
> I have not found a way that I like to expose some kind of grammar
> through securityfs that can be understood by usermode to parse the
> policy. Here's what I propose as a compromise:
>
> 1. I remove the unknown command behavior. This address your
> first point about inconsistent behaviors, and effectively removes the
> strict_parse sysctl (as it is always enabled).
>
> 2. I introduce a versioning system for the properties
> themselves. The valid set of properties and their versions
> can be found in securityfs, under say, ipe/config in a key=value
> format where `key` indicates the understood token, and `value`
> indicates their current version. For example:
>
> $ cat $SECURITYFS/ipe/config
> op=1
> action=1
> policy_name=1
> policy_version=1
> dmverity_signature=1
> dmverity_roothash=1
> boot_verified=1
The name ipe/config sounds like a file to configure IPE. Maybe something
like ipe/config_abi or ipe/config_grammar?
>
> if new syntax is introduced, the version number is increased.
>
> 3. The format of those versions are documented as part of
> the admin-guide around IPE. If user-mode at that point wants to rip
> the documentation formats and correlate with the versioning, then
> it fulfills the same functionality as above, with out the complexity
> around exposing a parsing grammar and interpreting it on-the-fly.
> Many of these are unlikely to move past version 1, however.
>
> Thoughts?
>
That seems reasonable.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code
On 2020/5/15 12:06, Matthew Wilcox wrote:
> On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote:
>> @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct
>> writeback_control *wbc)
>>
>> static sector_t swap_page_sector(struct page *page)
>> {
>> -return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9);
>> +return npage_to_sectors((sector_t)__page_file_index(page));
>
> If you make npage_to_sectors() a proper function instead of a macro,
> you can do the casting inside the function instead of in the callers
> (which is prone to bugs).
Oh, yes. __page_file_index(page) maybe called many times in marco, althouth
currently
it is not. So that, not all are suitable for page_to_sector(). And for this
example,
still need to use "<< PAGE_SECTORS_SHIFT".
>
> Also, this is a great example of why page_to_sector() was a better name
> than npage_to_sectors(). This function doesn't return a count of sectors,
> it returns a sector number within the swap device.
OK, so I will change to page_to_sector()/sector_to_page().
>
>
> .
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code
On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote:
> @@ -266,7 +266,7 @@ int swap_writepage(struct page *page, struct
> writeback_control *wbc)
>
> static sector_t swap_page_sector(struct page *page)
> {
> - return (sector_t)__page_file_index(page) << (PAGE_SHIFT - 9);
> + return npage_to_sectors((sector_t)__page_file_index(page));
If you make npage_to_sectors() a proper function instead of a macro,
you can do the casting inside the function instead of in the callers
(which is prone to bugs).
Also, this is a great example of why page_to_sector() was a better name
than npage_to_sectors(). This function doesn't return a count of sectors,
it returns a sector number within the swap device.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 07/10] block: use sectors_to_npage() and PAGE_SECTORS to clean up code
On Thu, May 07, 2020 at 03:50:57PM +0800, Zhen Lei wrote:
> +++ b/block/blk-settings.c
> @@ -150,7 +150,7 @@ void blk_queue_max_hw_sectors(struct request_queue *q,
> unsigned int max_hw_secto
> unsigned int max_sectors;
>
> if ((max_hw_sectors << 9) < PAGE_SIZE) {
> - max_hw_sectors = 1 << (PAGE_SHIFT - 9);
> + max_hw_sectors = PAGE_SECTORS;
Surely this should be:
if (max_hw_sectors < PAGE_SECTORS) {
max_hw_sectors = PAGE_SECTORS;
... no?
> - page = read_mapping_page(mapping,
> - (pgoff_t)(n >> (PAGE_SHIFT - 9)), NULL);
> + page = read_mapping_page(mapping, (pgoff_t)sectors_to_npage(n), NULL);
... again, get the type right, and you won't need the cast.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 00/10] clean up SECTOR related macros and sectors/pages conversions
Hi, all: It seems no one take care about these patches. But I think patch 1 is need. And the main discussion points of others is whether we should add sectors_to_npage()/npage_to_sectors() or keep PAGE_SECTORS_SHIFT. And which marco name do we prefer: PAGE_SECTORS vs SECTORS_PER_PAGE, PAGE_SECTORS_SHIFT vs SECTORS_PER_PAGE_SHIFT. Hi, Jens Axboe, Coly Li, Kent Overstreet,Alasdair Kergon. Mike Snitzer: Can you take a look at patch 1? On 2020/5/7 15:50, Zhen Lei wrote: > v1 --> v2: > As Matthew Wilcox's suggestion, add sectors_to_npage()/npage_to_sectors() > helpers to eliminate SECTORS_PER_PAGE_SHIFT, because it's quite hard to read. > In further, I also eliminated PAGE_SECTORS_SHIFT. > > I tried to eliminate all magic number "9" and "512", but it's too many, maybe > no one want to review it, so I gave up. In the process of searching, I found > the existing macro PAGE_SECTORS, it's equivalent to SECTORS_PER_PAGE. Because > PAGE_SECTORS was defined in include/linux/device-mapper.h, and > SECTORS_PER_PAGE > was defined in drivers/block/zram/zram_drv.h, so I discarded SECTORS_PER_PAGE, > althrough I prefer it so much. > > v1: > When I studied the code of mm/swap, I found "1 << (PAGE_SHIFT - 9)" appears > many times. So I try to clean up it. > > 1. Replace "1 << (PAGE_SHIFT - 9)" or similar with SECTORS_PER_PAGE > 2. Replace "PAGE_SHIFT - 9" with SECTORS_PER_PAGE_SHIFT > 3. Replace "9" with SECTOR_SHIFT > 4. Replace "512" with SECTOR_SIZE > > Zhen Lei (10): > block: move PAGE_SECTORS definition into > zram: abolish macro SECTORS_PER_PAGE > block: add sectors_to_npage()/npage_to_sectors() helpers > zram: abolish macro SECTORS_PER_PAGE_SHIFT > block: abolish macro PAGE_SECTORS_SHIFT > mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code > block: use sectors_to_npage() and PAGE_SECTORS to clean up code > md: use sectors_to_npage() and npage_to_sectors() to clean up code > md: use existing definition RESYNC_SECTORS > md: use PAGE_SECTORS to clean up code > > block/blk-settings.c | 6 +++--- > block/partitions/core.c | 5 ++--- > drivers/block/brd.c | 7 ++- > drivers/block/null_blk_main.c | 10 -- > drivers/block/zram/zram_drv.c | 8 > drivers/block/zram/zram_drv.h | 2 -- > drivers/md/bcache/util.h | 2 -- > drivers/md/dm-kcopyd.c| 2 +- > drivers/md/dm-table.c | 2 +- > drivers/md/md-bitmap.c| 16 > drivers/md/md.c | 6 +++--- > drivers/md/raid1.c| 10 +- > drivers/md/raid10.c | 28 ++-- > drivers/md/raid5-cache.c | 11 +-- > drivers/md/raid5.c| 4 ++-- > include/linux/blkdev.h| 7 +-- > include/linux/device-mapper.h | 1 - > mm/page_io.c | 4 ++-- > mm/swapfile.c | 12 ++-- > 19 files changed, 67 insertions(+), 76 deletions(-) > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code
On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote: > +++ b/mm/page_io.c > @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags, > > bio->bi_iter.bi_sector = map_swap_page(page, ); > bio_set_dev(bio, bdev); > - bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9; > + bio->bi_iter.bi_sector *= PAGE_SECTORS; > bio->bi_end_io = end_io; This just doesn't look right. Why is map_swap_page() returning a sector_t which isn't actually a sector_t? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 06/10] mm/swap: use npage_to_sectors() and PAGE_SECTORS to clean up code
On 2020/5/15 12:14, Matthew Wilcox wrote: > On Thu, May 07, 2020 at 03:50:56PM +0800, Zhen Lei wrote: >> +++ b/mm/page_io.c >> @@ -38,7 +38,7 @@ static struct bio *get_swap_bio(gfp_t gfp_flags, >> >> bio->bi_iter.bi_sector = map_swap_page(page, ); >> bio_set_dev(bio, bdev); >> -bio->bi_iter.bi_sector <<= PAGE_SHIFT - 9; >> +bio->bi_iter.bi_sector *= PAGE_SECTORS; >> bio->bi_end_io = end_io; > > This just doesn't look right. Why is map_swap_page() returning a sector_t > which isn't actually a sector_t? I try to understand map_swap_page(). Here maybe a bug. Otherwise, it would be better to add a temporary variable to cache the return value of map_swap_page(page, ). > > > . > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [dm:for-next 53/58] drivers/md/dm-zoned-metadata.c:779:3: error: implicit declaration of function 'export_uuid'
tree:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git
for-next
head: 6c871f63986b34c7768080259ddf5991c55ee385
commit: 70978208ec91d798066f4c291bc98ff914bea222 [53/58] dm zoned: metadata
version 2
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 70978208ec91d798066f4c291bc98ff914bea222
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross
ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
All errors (new ones prefixed by >>, old ones prefixed by <<):
drivers/md/dm-zoned-metadata.c: In function 'dmz_write_sb':
>> drivers/md/dm-zoned-metadata.c:779:3: error: implicit declaration of
>> function 'export_uuid' [-Werror=implicit-function-declaration]
779 | export_uuid(sb->dmz_uuid, >uuid);
| ^~~
drivers/md/dm-zoned-metadata.c: In function 'dmz_check_sb':
>> drivers/md/dm-zoned-metadata.c:1015:3: error: implicit declaration of
>> function 'import_uuid' [-Werror=implicit-function-declaration]
1015 | import_uuid(_uuid, sb->dmz_uuid);
| ^~~
cc1: some warnings being treated as errors
vim +/export_uuid +779 drivers/md/dm-zoned-metadata.c
761
762 /*
763 * Write super block of the specified metadata set.
764 */
765 static int dmz_write_sb(struct dmz_metadata *zmd, unsigned int set)
766 {
767 struct dmz_mblock *mblk = zmd->sb[set].mblk;
768 struct dmz_super *sb = zmd->sb[set].sb;
769 struct dmz_dev *dev = zmd->sb[set].dev;
770 sector_t sb_block;
771 u64 sb_gen = zmd->sb_gen + 1;
772 int ret;
773
774 sb->magic = cpu_to_le32(DMZ_MAGIC);
775
776 sb->version = cpu_to_le32(zmd->sb_version);
777 if (zmd->sb_version > 1) {
778 BUILD_BUG_ON(UUID_SIZE != 16);
> 779 export_uuid(sb->dmz_uuid, >uuid);
780 memcpy(sb->dmz_label, zmd->label, BDEVNAME_SIZE);
781 export_uuid(sb->dev_uuid, >uuid);
782 }
783
784 sb->gen = cpu_to_le64(sb_gen);
785
786 /*
787 * The metadata always references the absolute block address,
788 * ie relative to the entire block range, not the per-device
789 * block address.
790 */
791 sb_block = zmd->sb[set].zone->id << zmd->zone_nr_blocks_shift;
792 sb->sb_block = cpu_to_le64(sb_block);
793 sb->nr_meta_blocks = cpu_to_le32(zmd->nr_meta_blocks);
794 sb->nr_reserved_seq = cpu_to_le32(zmd->nr_reserved_seq);
795 sb->nr_chunks = cpu_to_le32(zmd->nr_chunks);
796
797 sb->nr_map_blocks = cpu_to_le32(zmd->nr_map_blocks);
798 sb->nr_bitmap_blocks = cpu_to_le32(zmd->nr_bitmap_blocks);
799
800 sb->crc = 0;
801 sb->crc = cpu_to_le32(crc32_le(sb_gen, (unsigned char *)sb,
DMZ_BLOCK_SIZE));
802
803 ret = dmz_rdwr_block(dev, REQ_OP_WRITE, zmd->sb[set].block,
804 mblk->page);
805 if (ret == 0)
806 ret = blkdev_issue_flush(dev->bdev, GFP_NOIO, NULL);
807
808 return ret;
809 }
810
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
