diff for duplicates of <20150128002203.GB25828@blaptop> diff --git a/a/1.txt b/N1/1.txt index 4b498da..02db1f7 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -324,3 +324,230 @@ On Wed, Jan 28, 2015 at 09:15:27AM +0900, Minchan Kim wrote: > Another idea is to use kick_all_cpus_sync, not srcu. > With that, we don't need to add more instruction in rw path. > I will try it. + +>From 560478040d2e08c61796e67d0c3ee519ae67ac0f Mon Sep 17 00:00:00 2001 +From: Minchan Kim <minchan@kernel.org> +Date: Mon, 26 Jan 2015 14:34:10 +0900 +Subject: [PATCH] zram: remove init_lock in zram_make_request + +Admin could reset zram during I/O operation going on so we have +used zram->init_lock as read-side lock in I/O path to prevent +sudden zram meta freeing. + +However, the init_lock is really troublesome. +We can't do call zram_meta_alloc under init_lock due to lockdep splat +because zram_rw_page is one of the function under reclaim path and +hold it as read_lock while other places in process context hold it +as write_lock. So, we have used allocation out of the lock to avoid +lockdep warn but it's not good for readability and finally, I met +another lockdep splat between init_lock and cpu_hotpulug from +kmem_cache_destroy during wokring zsmalloc compaction. :( + +Yes, the ideal is to remove horrible init_lock of zram in rw path. +This patch removes it in rw path and instead, use kick_all_cpus_sync +and a bool init_done variable to check initialization done with +smp_[wmb|rmb]. + +Upon kick_all_cpus_sync returns, any CPU cannot access zram meta +any more due to init_done in zram_make_request so it's safe to +free meta. So, finally, we avoids init_lock in reclaim context +so we are free for deadlock. + +Signed-off-by: Minchan Kim <minchan@kernel.org> +--- + drivers/block/zram/zram_drv.c | 70 +++++++++++++++++++++++++------------------ + drivers/block/zram/zram_drv.h | 2 ++ + 2 files changed, 43 insertions(+), 29 deletions(-) + +diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c +index a598ada817f0..404602b1932e 100644 +--- a/drivers/block/zram/zram_drv.c ++++ b/drivers/block/zram/zram_drv.c +@@ -53,9 +53,16 @@ static ssize_t name##_show(struct device *d, \ + } \ + static DEVICE_ATTR_RO(name); + +-static inline int init_done(struct zram *zram) ++static inline bool init_done(struct zram *zram) + { +- return zram->meta != NULL; ++ /* ++ * init_done can be used without holding zram->init_lock in ++ * read/write handler(ie, zram_make_request) but we should make sure ++ * that zram->init_done should set up after meta initialization is ++ * done. Look at disksize_store. ++ */ ++ smp_rmb(); ++ return zram->init_done; + } + + static inline struct zram *dev_to_zram(struct device *dev) +@@ -726,11 +733,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity) + return; + } + +- zcomp_destroy(zram->comp); + zram->max_comp_streams = 1; + +- zram_meta_free(zram->meta); +- zram->meta = NULL; + /* Reset stats */ + memset(&zram->stats, 0, sizeof(zram->stats)); + +@@ -738,8 +742,16 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity) + if (reset_capacity) + set_capacity(zram->disk, 0); + ++ zram->init_done = false; ++ /* don't need smp_wmb because kick_all_cpus_sync does */ ++ kick_all_cpus_sync(); ++ /* ++ * From now on, any read/write cannot access zram meta data ++ * by init_done in the handler. ++ */ ++ zram_meta_free(zram->meta); ++ zcomp_destroy(zram->comp); + up_write(&zram->init_lock); +- + /* + * Revalidate disk out of the init_lock to avoid lockdep splat. + * It's okay because disk's capacity is protected by init_lock +@@ -762,10 +774,19 @@ static ssize_t disksize_store(struct device *dev, + if (!disksize) + return -EINVAL; + ++ down_write(&zram->init_lock); ++ if (init_done(zram)) { ++ pr_info("Cannot change disksize for initialized device\n"); ++ up_write(&zram->init_lock); ++ return -EBUSY; ++ } ++ + disksize = PAGE_ALIGN(disksize); + meta = zram_meta_alloc(zram->disk->first_minor, disksize); +- if (!meta) ++ if (!meta) { ++ up_write(&zram->init_lock); + return -ENOMEM; ++ } + + comp = zcomp_create(zram->compressor, zram->max_comp_streams); + if (IS_ERR(comp)) { +@@ -775,17 +796,17 @@ static ssize_t disksize_store(struct device *dev, + goto out_free_meta; + } + +- down_write(&zram->init_lock); +- if (init_done(zram)) { +- pr_info("Cannot change disksize for initialized device\n"); +- err = -EBUSY; +- goto out_destroy_comp; +- } +- + zram->meta = meta; + zram->comp = comp; + zram->disksize = disksize; + set_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT); ++ /* ++ * Store operation of struct zram fields should complete ++ * before init_done set up because zram_bvec_rw doesn't ++ * hold an zram->init_lock. ++ */ ++ smp_wmb(); ++ zram->init_done = true; + up_write(&zram->init_lock); + + /* +@@ -797,10 +818,8 @@ static ssize_t disksize_store(struct device *dev, + + return len; + +-out_destroy_comp: +- up_write(&zram->init_lock); +- zcomp_destroy(comp); + out_free_meta: ++ up_write(&zram->init_lock); + zram_meta_free(meta); + return err; + } +@@ -907,7 +926,6 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) + { + struct zram *zram = queue->queuedata; + +- down_read(&zram->init_lock); + if (unlikely(!init_done(zram))) + goto error; + +@@ -918,12 +936,10 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio) + } + + __zram_make_request(zram, bio); +- up_read(&zram->init_lock); + + return; + + error: +- up_read(&zram->init_lock); + bio_io_error(bio); + } + +@@ -951,17 +967,16 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, + struct bio_vec bv; + + zram = bdev->bd_disk->private_data; ++ ++ /* This should be another patch */ ++ if (unlikely(!init_done(zram))) ++ return -EIO; ++ + if (!valid_io_request(zram, sector, PAGE_SIZE)) { + atomic64_inc(&zram->stats.invalid_io); + return -EINVAL; + } + +- down_read(&zram->init_lock); +- if (unlikely(!init_done(zram))) { +- err = -EIO; +- goto out_unlock; +- } +- + index = sector >> SECTORS_PER_PAGE_SHIFT; + offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT; + +@@ -970,8 +985,6 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector, + bv.bv_offset = 0; + + err = zram_bvec_rw(zram, &bv, index, offset, rw); +-out_unlock: +- up_read(&zram->init_lock); + /* + * If I/O fails, just return error(ie, non-zero) without + * calling page_endio. +@@ -1125,7 +1138,6 @@ static void destroy_device(struct zram *zram) + + del_gendisk(zram->disk); + put_disk(zram->disk); +- + blk_cleanup_queue(zram->queue); + } + +diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h +index e492f6bf11f1..dca265654285 100644 +--- a/drivers/block/zram/zram_drv.h ++++ b/drivers/block/zram/zram_drv.h +@@ -107,6 +107,8 @@ struct zram { + + /* Prevent concurrent execution of device init, reset and R/W request */ + struct rw_semaphore init_lock; ++ bool init_done; ++ + /* + * This is the limit on amount of *uncompressed* worth of data + * we can store in a disk. +-- +1.9.1 + + +-- +Kind regards, +Minchan Kim diff --git a/a/content_digest b/N1/content_digest index 6d8c41b..72f4e5c 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -345,6 +345,233 @@ "> \n" "> Another idea is to use kick_all_cpus_sync, not srcu.\n" "> With that, we don't need to add more instruction in rw path.\n" - > I will try it. + "> I will try it.\n" + "\n" + ">From 560478040d2e08c61796e67d0c3ee519ae67ac0f Mon Sep 17 00:00:00 2001\n" + "From: Minchan Kim <minchan@kernel.org>\n" + "Date: Mon, 26 Jan 2015 14:34:10 +0900\n" + "Subject: [PATCH] zram: remove init_lock in zram_make_request\n" + "\n" + "Admin could reset zram during I/O operation going on so we have\n" + "used zram->init_lock as read-side lock in I/O path to prevent\n" + "sudden zram meta freeing.\n" + "\n" + "However, the init_lock is really troublesome.\n" + "We can't do call zram_meta_alloc under init_lock due to lockdep splat\n" + "because zram_rw_page is one of the function under reclaim path and\n" + "hold it as read_lock while other places in process context hold it\n" + "as write_lock. So, we have used allocation out of the lock to avoid\n" + "lockdep warn but it's not good for readability and finally, I met\n" + "another lockdep splat between init_lock and cpu_hotpulug from\n" + "kmem_cache_destroy during wokring zsmalloc compaction. :(\n" + "\n" + "Yes, the ideal is to remove horrible init_lock of zram in rw path.\n" + "This patch removes it in rw path and instead, use kick_all_cpus_sync\n" + "and a bool init_done variable to check initialization done with\n" + "smp_[wmb|rmb].\n" + "\n" + "Upon kick_all_cpus_sync returns, any CPU cannot access zram meta\n" + "any more due to init_done in zram_make_request so it's safe to\n" + "free meta. So, finally, we avoids init_lock in reclaim context\n" + "so we are free for deadlock.\n" + "\n" + "Signed-off-by: Minchan Kim <minchan@kernel.org>\n" + "---\n" + " drivers/block/zram/zram_drv.c | 70 +++++++++++++++++++++++++------------------\n" + " drivers/block/zram/zram_drv.h | 2 ++\n" + " 2 files changed, 43 insertions(+), 29 deletions(-)\n" + "\n" + "diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c\n" + "index a598ada817f0..404602b1932e 100644\n" + "--- a/drivers/block/zram/zram_drv.c\n" + "+++ b/drivers/block/zram/zram_drv.c\n" + "@@ -53,9 +53,16 @@ static ssize_t name##_show(struct device *d,\t\t\\\n" + " }\t\t\t\t\t\t\t\t\t\\\n" + " static DEVICE_ATTR_RO(name);\n" + " \n" + "-static inline int init_done(struct zram *zram)\n" + "+static inline bool init_done(struct zram *zram)\n" + " {\n" + "-\treturn zram->meta != NULL;\n" + "+\t/*\n" + "+\t * init_done can be used without holding zram->init_lock in\n" + "+\t * read/write handler(ie, zram_make_request) but we should make sure\n" + "+\t * that zram->init_done should set up after meta initialization is\n" + "+\t * done. Look at disksize_store.\n" + "+\t */\n" + "+\tsmp_rmb();\n" + "+\treturn zram->init_done;\n" + " }\n" + " \n" + " static inline struct zram *dev_to_zram(struct device *dev)\n" + "@@ -726,11 +733,8 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)\n" + " \t\treturn;\n" + " \t}\n" + " \n" + "-\tzcomp_destroy(zram->comp);\n" + " \tzram->max_comp_streams = 1;\n" + " \n" + "-\tzram_meta_free(zram->meta);\n" + "-\tzram->meta = NULL;\n" + " \t/* Reset stats */\n" + " \tmemset(&zram->stats, 0, sizeof(zram->stats));\n" + " \n" + "@@ -738,8 +742,16 @@ static void zram_reset_device(struct zram *zram, bool reset_capacity)\n" + " \tif (reset_capacity)\n" + " \t\tset_capacity(zram->disk, 0);\n" + " \n" + "+\tzram->init_done = false;\n" + "+\t/* don't need smp_wmb because kick_all_cpus_sync does */\n" + "+\tkick_all_cpus_sync();\n" + "+\t/*\n" + "+\t * From now on, any read/write cannot access zram meta data\n" + "+\t * by init_done in the handler.\n" + "+\t */\n" + "+\tzram_meta_free(zram->meta);\n" + "+\tzcomp_destroy(zram->comp);\n" + " \tup_write(&zram->init_lock);\n" + "-\n" + " \t/*\n" + " \t * Revalidate disk out of the init_lock to avoid lockdep splat.\n" + " \t * It's okay because disk's capacity is protected by init_lock\n" + "@@ -762,10 +774,19 @@ static ssize_t disksize_store(struct device *dev,\n" + " \tif (!disksize)\n" + " \t\treturn -EINVAL;\n" + " \n" + "+\tdown_write(&zram->init_lock);\n" + "+\tif (init_done(zram)) {\n" + "+\t\tpr_info(\"Cannot change disksize for initialized device\\n\");\n" + "+\t\tup_write(&zram->init_lock);\n" + "+\t\treturn -EBUSY;\n" + "+\t}\n" + "+\n" + " \tdisksize = PAGE_ALIGN(disksize);\n" + " \tmeta = zram_meta_alloc(zram->disk->first_minor, disksize);\n" + "-\tif (!meta)\n" + "+\tif (!meta) {\n" + "+\t\tup_write(&zram->init_lock);\n" + " \t\treturn -ENOMEM;\n" + "+\t}\n" + " \n" + " \tcomp = zcomp_create(zram->compressor, zram->max_comp_streams);\n" + " \tif (IS_ERR(comp)) {\n" + "@@ -775,17 +796,17 @@ static ssize_t disksize_store(struct device *dev,\n" + " \t\tgoto out_free_meta;\n" + " \t}\n" + " \n" + "-\tdown_write(&zram->init_lock);\n" + "-\tif (init_done(zram)) {\n" + "-\t\tpr_info(\"Cannot change disksize for initialized device\\n\");\n" + "-\t\terr = -EBUSY;\n" + "-\t\tgoto out_destroy_comp;\n" + "-\t}\n" + "-\n" + " \tzram->meta = meta;\n" + " \tzram->comp = comp;\n" + " \tzram->disksize = disksize;\n" + " \tset_capacity(zram->disk, zram->disksize >> SECTOR_SHIFT);\n" + "+\t/*\n" + "+\t * Store operation of struct zram fields should complete\n" + "+\t * before init_done set up because zram_bvec_rw doesn't\n" + "+\t * hold an zram->init_lock.\n" + "+\t */\n" + "+\tsmp_wmb();\n" + "+\tzram->init_done = true;\n" + " \tup_write(&zram->init_lock);\n" + " \n" + " \t/*\n" + "@@ -797,10 +818,8 @@ static ssize_t disksize_store(struct device *dev,\n" + " \n" + " \treturn len;\n" + " \n" + "-out_destroy_comp:\n" + "-\tup_write(&zram->init_lock);\n" + "-\tzcomp_destroy(comp);\n" + " out_free_meta:\n" + "+\tup_write(&zram->init_lock);\n" + " \tzram_meta_free(meta);\n" + " \treturn err;\n" + " }\n" + "@@ -907,7 +926,6 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)\n" + " {\n" + " \tstruct zram *zram = queue->queuedata;\n" + " \n" + "-\tdown_read(&zram->init_lock);\n" + " \tif (unlikely(!init_done(zram)))\n" + " \t\tgoto error;\n" + " \n" + "@@ -918,12 +936,10 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)\n" + " \t}\n" + " \n" + " \t__zram_make_request(zram, bio);\n" + "-\tup_read(&zram->init_lock);\n" + " \n" + " \treturn;\n" + " \n" + " error:\n" + "-\tup_read(&zram->init_lock);\n" + " \tbio_io_error(bio);\n" + " }\n" + " \n" + "@@ -951,17 +967,16 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,\n" + " \tstruct bio_vec bv;\n" + " \n" + " \tzram = bdev->bd_disk->private_data;\n" + "+\n" + "+\t/* This should be another patch */\n" + "+\tif (unlikely(!init_done(zram)))\n" + "+\t\treturn -EIO;\n" + "+\n" + " \tif (!valid_io_request(zram, sector, PAGE_SIZE)) {\n" + " \t\tatomic64_inc(&zram->stats.invalid_io);\n" + " \t\treturn -EINVAL;\n" + " \t}\n" + " \n" + "-\tdown_read(&zram->init_lock);\n" + "-\tif (unlikely(!init_done(zram))) {\n" + "-\t\terr = -EIO;\n" + "-\t\tgoto out_unlock;\n" + "-\t}\n" + "-\n" + " \tindex = sector >> SECTORS_PER_PAGE_SHIFT;\n" + " \toffset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;\n" + " \n" + "@@ -970,8 +985,6 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,\n" + " \tbv.bv_offset = 0;\n" + " \n" + " \terr = zram_bvec_rw(zram, &bv, index, offset, rw);\n" + "-out_unlock:\n" + "-\tup_read(&zram->init_lock);\n" + " \t/*\n" + " \t * If I/O fails, just return error(ie, non-zero) without\n" + " \t * calling page_endio.\n" + "@@ -1125,7 +1138,6 @@ static void destroy_device(struct zram *zram)\n" + " \n" + " \tdel_gendisk(zram->disk);\n" + " \tput_disk(zram->disk);\n" + "-\n" + " \tblk_cleanup_queue(zram->queue);\n" + " }\n" + " \n" + "diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h\n" + "index e492f6bf11f1..dca265654285 100644\n" + "--- a/drivers/block/zram/zram_drv.h\n" + "+++ b/drivers/block/zram/zram_drv.h\n" + "@@ -107,6 +107,8 @@ struct zram {\n" + " \n" + " \t/* Prevent concurrent execution of device init, reset and R/W request */\n" + " \tstruct rw_semaphore init_lock;\n" + "+\tbool init_done;\n" + "+\n" + " \t/*\n" + " \t * This is the limit on amount of *uncompressed* worth of data\n" + " \t * we can store in a disk.\n" + "-- \n" + "1.9.1\n" + "\n" + "\n" + "-- \n" + "Kind regards,\n" + Minchan Kim -e919f463fe4dafd62f1daaf1e49de1b1f4a6b6cd831f6df9ec82fcf88165470f +7d781ec6c978e50d0518ff02b1c398e62b33d8bdeef2756572fd9ba1bdef3031
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.