Enable lock context analysis in drivers/block/
Subsystems (9)
cover Enable lock context analysis in drivers/block/
Hi Jens,
This patch series enables lock context analysis in all block drivers in the drivers/block/ directory. Please consider these patches for the next merge window.
Thanks,
Bart.
Bart Van Assche (27):
aoe: Enable lock context analysis
drbd: Remove "extern" from function declarations
drbd: Retain one _get_ldev_if_state() implementation
drbd: Remove the get_ldev_if_state() macro
drbd: Remove the 'local' lock context
drbd: Simplify the bitmap locking functions.
drbd: Move two declarations
drbd: Pass 'resource' directly to complete_conflicting_writes()
drbd: Split drbd_nl_get_connections_dumpit()
drbd: Make a mutex_unlock() call unconditional
drbd: Split drbd_req_state()
drbd: Convert drbd_req_state() to unconditional locking
drbd: Annotate drbd_bm_{lock,unlock}()
drbd: Enable lock context analysis
loop: Split loop_change_fd()
loop: Split loop_configure()
loop: Remove the "bool global" function argument
loop: Add lock context annotations
mtip32: Enable lock context analysis
nbd: Enable lock context analysis
null_blk: Enable lock context analysis
rbd: Enable lock context analysis
ublk: Enable lock context analysis
xen-blkback: Enable lock context analysis
zram: Enable lock context analysis
rnbd: Enable lock context analysis
block: Enable lock context analysis for all block drivers
drivers/block/Makefile | 2 + drivers/block/aoe/Makefile | 2 + drivers/block/aoe/aoecmd.c | 1 + drivers/block/drbd/Makefile | 3 + drivers/block/drbd/drbd_bitmap.c | 43 +- drivers/block/drbd/drbd_config.h | 2 +- drivers/block/drbd/drbd_int.h | 594 +++++++++++++------------ drivers/block/drbd/drbd_interval.h | 11 +- drivers/block/drbd/drbd_main.c | 41 +- drivers/block/drbd/drbd_nl.c | 117 ++--- drivers/block/drbd/drbd_receiver.c | 27 +- drivers/block/drbd/drbd_req.c | 10 +- drivers/block/drbd/drbd_req.h | 24 +- drivers/block/drbd/drbd_state.c | 50 ++- drivers/block/drbd/drbd_state.h | 41 +- drivers/block/drbd/drbd_state_change.h | 32 +- drivers/block/drbd/drbd_worker.c | 6 +- drivers/block/loop.c | 251 ++++++----- drivers/block/mtip32xx/Makefile | 2 + drivers/block/nbd.c | 3 + drivers/block/null_blk/Makefile | 2 + drivers/block/null_blk/main.c | 12 +- drivers/block/null_blk/zoned.c | 2 + drivers/block/rbd.c | 8 + drivers/block/rnbd/Makefile | 2 + drivers/block/ublk_drv.c | 6 + drivers/block/xen-blkback/Makefile | 3 + drivers/block/zram/Makefile | 2 + drivers/block/zram/zcomp.c | 3 +- drivers/block/zram/zcomp.h | 6 +- 30 files changed, 700 insertions(+), 608 deletions(-)
PATCH 1/27 aoe: Enable lock context analysis
Add a missing __must_hold() annotation. Enable lock context analysis in the Makefile.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/aoe/Makefile | 2 ++ drivers/block/aoe/aoecmd.c | 1 + 2 files changed, 3 insertions(+)
drivers/block/aoe/Makefile +2 -0
@@ -3,5 +3,7 @@ # Makefile for ATA over Ethernet # +CONTEXT_ANALYSIS := y + obj-$(CONFIG_ATA_OVER_ETH) += aoe.o aoe-y := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.o
drivers/block/aoe/aoecmd.c +1 -0
@@ -1193,6 +1193,7 @@ noskb: if (buf)*/ static int ktio (int id) + __must_hold(&iocq[id].lock) {struct frame *f;struct list_head *pos;
Re: [PATCH 01/27] aoe: Enable lock context analysis
Quoted 5 lines · Bart Van Assche
Add a missing
__must_hold()annotation. Enable lock context analysis in theMakefile.Reviewed-by Christoph Hellwig <[email protected]>Signed-off-by Bart Van Assche <[email protected]>
Looks good,
Reviewed-by Md Haris Iqbal <[email protected]>
Quoted 30 lines
--- drivers/block/aoe/Makefile | 2 ++ drivers/block/aoe/aoecmd.c | 1 + 2 files changed, 3 insertions(+)drivers/block/aoe/Makefile +2
-0@@ -3,5 +3,7 @@ # Makefile for ATA over Ethernet # +CONTEXT_ANALYSIS := y + obj-$(CONFIG_ATA_OVER_ETH) += aoe.o aoe-y := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.odrivers/block/aoe/aoecmd.c +1
-0@@ -1193,6 +1193,7 @@ noskb: if (buf)*/ static int ktio (int id) + __must_hold(&iocq[id].lock) {struct frame *f;struct list_head *pos;
PATCH 2/27 drbd: Remove "extern" from function declarations
In reply to Bart Van AsscheThe "extern" keyword is superfluous when declaring a function. Remove that keyword from function definitions to improve consistency with the rest of the kernel source code.
Suggested-by Christoph Hellwig <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_config.h | 2 +- drivers/block/drbd/drbd_int.h | 576 +++++++++++++------------ drivers/block/drbd/drbd_interval.h | 11 +- drivers/block/drbd/drbd_req.h | 24 +- drivers/block/drbd/drbd_state.h | 35 +- drivers/block/drbd/drbd_state_change.h | 32 +- 6 files changed, 346 insertions(+), 334 deletions(-)
drivers/block/drbd/drbd_config.h +1 -1
@@ -7,7 +7,7 @@#ifndef DRBD_CONFIG_H #define DRBD_CONFIG_H -extern const char *drbd_buildtag (void ); +const char *drbd_buildtag (void );#define REL_VERSION "8.4.11"#define PRO_VERSION_MIN 86
drivers/block/drbd/drbd_int.h +301 -275
706 lines not on this page · raw message
drivers/block/drbd/drbd_interval.h +5 -6
@@ -29,13 +29,12 @@ static inline bool drbd_interval_empty(struct drbd_interval *i)return RB_EMPTY_NODE (&i->rb); } -extern bool drbd_insert_interval (struct rb_root *,struct drbd_interval *); -extern bool drbd_contains_interval (struct rb_root *,sector_t , -struct drbd_interval *); -extern void drbd_remove_interval (struct rb_root *,struct drbd_interval *); -extern struct drbd_interval *drbd_find_overlap (struct rb_root *,sector_t , +bool drbd_insert_interval (struct rb_root *,struct drbd_interval *); +bool drbd_contains_interval (struct rb_root *,sector_t ,struct drbd_interval *); +void drbd_remove_interval (struct rb_root *,struct drbd_interval *); +struct drbd_interval *drbd_find_overlap (struct rb_root *,sector_t ,unsigned int ); -extern struct drbd_interval *drbd_next_overlap (struct drbd_interval *,sector_t , +struct drbd_interval *drbd_next_overlap (struct drbd_interval *,sector_t ,unsigned int );#define drbd_for_each_overlap (i, root, sector, size) \
drivers/block/drbd/drbd_req.h +11 -13
@@ -264,20 +264,18 @@ struct bio_and_error {int error; }; -extern void start_new_tl_epoch (struct drbd_connection *connection); -extern void drbd_req_destroy (struct kref *kref); -extern int __req_mod (struct drbd_request *req,enum drbd_req_event what, -struct drbd_peer_device *peer_device, -struct bio_and_error *m); -extern void complete_master_bio (struct drbd_device *device, -struct bio_and_error *m); -extern void request_timer_fn (struct timer_list *t); -extern void tl_restart (struct drbd_connection *connection,enum drbd_req_event what); -extern void _tl_restart (struct drbd_connection *connection,enum drbd_req_event what); -extern void tl_abort_disk_io (struct drbd_device *device); +void start_new_tl_epoch (struct drbd_connection *connection); +void drbd_req_destroy (struct kref *kref); +int __req_mod (struct drbd_request *req,enum drbd_req_event what, +struct drbd_peer_device *peer_device,struct bio_and_error *m); +void complete_master_bio (struct drbd_device *device,struct bio_and_error *m); +void request_timer_fn (struct timer_list *t); +void tl_restart (struct drbd_connection *connection,enum drbd_req_event what); +void _tl_restart (struct drbd_connection *connection,enum drbd_req_event what); +void tl_abort_disk_io (struct drbd_device *device);/* this is in drbd_main.c */ -extern void drbd_restart_request (struct drbd_request *req); +void drbd_restart_request (struct drbd_request *req);/* use this if you don't want to deal with calling complete_master_bio() * outside the spinlock, e.g. when walking some list on cleanup. */
@@ -319,6 +317,6 @@ static inline int req_mod(struct drbd_request *req, return rv; } -extern bool drbd_should_do_remote (union drbd_dev_state ); +bool drbd_should_do_remote (union drbd_dev_state );#endif
drivers/block/drbd/drbd_state.h +16 -19
@@ -112,26 +112,23 @@ union drbd_dev_state {unsigned int i; }; -extern enum drbd_state_rv drbd_change_state (struct drbd_device *device, -enum chg_state_flags f, -union drbd_state mask, -union drbd_state val); -extern void drbd_force_state (struct drbd_device *,union drbd_state , -union drbd_state ); -extern enum drbd_state_rv _drbd_request_state (struct drbd_device *, -union drbd_state , -union drbd_state , -enum chg_state_flags ); - -extern enum drbd_state_rv +enum drbd_state_rv drbd_change_state (struct drbd_device *device, +enum chg_state_flags f, +union drbd_state mask, +union drbd_state val); +void drbd_force_state (struct drbd_device *,union drbd_state ,union drbd_state ); +enum drbd_state_rv _drbd_request_state (struct drbd_device *,union drbd_state , +union drbd_state ,enum chg_state_flags ); + +enum drbd_state_rv _drbd_request_state_holding_state_mutex (struct drbd_device *,union drbd_state ,union drbd_state ,enum chg_state_flags ); -extern enum drbd_state_rv _drbd_set_state (struct drbd_device *,union drbd_state , -enum chg_state_flags , -struct completion *done); -extern void print_st_err (struct drbd_device *,union drbd_state , -union drbd_state ,enum drbd_state_rv ); +enum drbd_state_rv _drbd_set_state (struct drbd_device *,union drbd_state , +enum chg_state_flags , +struct completion *done); +void print_st_err (struct drbd_device *,union drbd_state ,union drbd_state , +enum drbd_state_rv ); enum drbd_state_rv _conn_request_state(struct drbd_connection *connection, union drbd_state mask, union drbd_state val,
@@ -141,8 +138,8 @@ enum drbd_state_rv conn_request_state(struct drbd_connection *connection, union drbd_state mask, union drbd_state val, enum chg_state_flags flags); -extern void drbd_resume_al (struct drbd_device *device); -extern bool conn_all_vols_unconf (struct drbd_connection *connection); +void drbd_resume_al (struct drbd_device *device); +bool conn_all_vols_unconf (struct drbd_connection *connection);/** * drbd_request_state() - Request a state change
drivers/block/drbd/drbd_state_change.h +12 -20
@@ -40,25 +40,17 @@ struct drbd_state_change { struct drbd_peer_device_state_change *peer_devices; }; -extern struct drbd_state_change *remember_old_state (struct drbd_resource *,gfp_t ); -extern void copy_old_to_new_state_change (struct drbd_state_change *); -extern void forget_state_change (struct drbd_state_change *); - -extern int notify_resource_state_change (struct sk_buff *, -unsigned int , -void *, -enum drbd_notification_type type); -extern int notify_connection_state_change (struct sk_buff *, -unsigned int , -void *, -enum drbd_notification_type type); -extern int notify_device_state_change (struct sk_buff *, -unsigned int , -void *, -enum drbd_notification_type type); -extern int notify_peer_device_state_change (struct sk_buff *, -unsigned int , -void *, -enum drbd_notification_type type); +struct drbd_state_change *remember_old_state (struct drbd_resource *,gfp_t ); +void copy_old_to_new_state_change (struct drbd_state_change *); +void forget_state_change (struct drbd_state_change *); + +int notify_resource_state_change (struct sk_buff *,unsigned int ,void *, +enum drbd_notification_type type); +int notify_connection_state_change (struct sk_buff *,unsigned int ,void *, +enum drbd_notification_type type); +int notify_device_state_change (struct sk_buff *,unsigned int ,void *, +enum drbd_notification_type type); +int notify_peer_device_state_change (struct sk_buff *,unsigned int ,void *, +enum drbd_notification_type type);#endif /* DRBD_STATE_CHANGE_H */
Re: [PATCH 02/27] drbd: Remove "extern" from function declarations
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 3/27 drbd: Retain one _get_ldev_if_state() implementation
In reply to Bart Van AsscheThere are two slightly different _get_ldev_if_state() implementations in the DRBD source code. Keep the version that is used. This does not affect C=1 / C=2 builds because lock context checking is performed by Clang instead of sparse since commit 5b63d0ae94cc ("compiler-context-
analysis: Remove Sparse support").
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_int.h | 7 ++----- drivers/block/drbd/drbd_main.c | 19 ------------------- 2 files changed, 2 insertions(+), 24 deletions(-)
drivers/block/drbd/drbd_int.h +2 -5
@@ -2033,8 +2033,8 @@ static inline void put_ldev(struct drbd_device *device) } } -#ifndef __CHECKER__ -static inline int _get_ldev_if_state (struct drbd_device *device,enum drbd_disk_state mins) +static inline int _get_ldev_if_state (struct drbd_device *device, +enum drbd_disk_state mins) {int io_allowed;
@@ -2048,9 +2048,6 @@ static inline int _get_ldev_if_state(struct drbd_device *device, enum drbd_disk_put_ldev (device);return io_allowed; } -#else -int _get_ldev_if_state (struct drbd_device *device,enum drbd_disk_state mins); -#endif /* this throttles on-the-fly application requests * according to max_buffers settings;
drivers/block/drbd/drbd_main.c +0 -19
@@ -128,25 +128,6 @@ static const struct block_device_operations drbd_ops = { .release = drbd_release, }; -#ifdef __CHECKER__ -/* When checking with sparse, and this is an inline function, sparse will -give tons of false positives. When this is a real functions sparse works. -*/ -int _get_ldev_if_state (struct drbd_device *device,enum drbd_disk_state mins) -{ -int io_allowed; - -atomic_inc (&device->local_cnt); - io_allowed = (device->state.disk >= mins); -if (!io_allowed) { -if (atomic_dec_and_test (&device->local_cnt)) -wake_up (&device->misc_wait); - } -return io_allowed; -} - -#endif -/** * tl_release() - mark as BARRIER_ACKED all requests in the corresponding transfer log epoch * @connection: DRBD connection.
Re: [PATCH 03/27] drbd: Retain one _get_ldev_if_state() implementation
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 4/27 drbd: Remove the get_ldev_if_state() macro
In reply to Bart Van AsscheThe get_ldev_if_state() macro has been introduced because sparse does not support something like __cond_acquires(). Remove this macro and rename _get_ldev_if_state() into get_ldev_if_state().
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_int.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/block/drbd/drbd_int.h +2 -5
@@ -2002,9 +2002,6 @@ static inline bool is_sync_state(enum drbd_conns connection_state)* * You have to call put_ldev() when finished working with device->ldev. */ -#define get_ldev_if_state (_device, _min_state) \ - (_get_ldev_if_state((_device), (_min_state)) ? \ - ({ __acquire(x); true; }) : false)#define get_ldev (_device) get_ldev_if_state(_device, D_INCONSISTENT)static inline void put_ldev (struct drbd_device *device)
@@ -2033,8 +2030,8 @@ static inline void put_ldev(struct drbd_device *device) } } -static inline int _get_ldev_if_state (struct drbd_device *device, -enum drbd_disk_state mins) +static inline int get_ldev_if_state (struct drbd_device *device, +enum drbd_disk_state mins) {int io_allowed;
Re: [PATCH 04/27] drbd: Remove the get_ldev_if_state() macro
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
Quoted 5 lines
drivers/block/drbd/drbd_int.h +0
-1- ({ __acquire(x); true; }) : false)#define get_ldev (_device) get_ldev_if_state(_device, D_INCONSISTENT)static inline void put_ldev (struct drbd_device *device)
drivers/block/drbd/drbd_int.h static inline void put_ldev(struct drbd_device *device) · line 2037
Quoted 10 lines
drivers/block/drbd/drbd_int.h +2
-2@@ -2033,8 +2030,8 @@ static inline void put_ldev(struct drbd_device *device) } } -static inline int _get_ldev_if_state (struct drbd_device *device, -enum drbd_disk_state mins) +static inline int get_ldev_if_state (struct drbd_device *device, +enum drbd_disk_state mins) {int io_allowed;
---end quoted text---
PATCH 5/27 drbd: Remove the 'local' lock context
In reply to Bart Van AsscheThe 'local' lock context represents code sections between get_ldev() and put_ldev() calls. Between these two calls the device->ldev pointer is valid. Since there are multiple functions with unpaired get_ldev() / put_ldev() calls, remove this lock context.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_bitmap.c | 17 +++++----- drivers/block/drbd/drbd_int.h | 50 ++++++++++++------------------ drivers/block/drbd/drbd_main.c | 20 ++++++------ drivers/block/drbd/drbd_nl.c | 7 +++-- drivers/block/drbd/drbd_receiver.c | 21 +++++++------ drivers/block/drbd/drbd_worker.c | 6 ++-- 6 files changed, 55 insertions(+), 66 deletions(-)
drivers/block/drbd/drbd_bitmap.c +9 -8
@@ -987,7 +987,7 @@ static inline sector_t drbd_md_last_bitmap_sector(struct drbd_backing_dev *bdev) } } -static void bm_page_io_async (struct drbd_bm_aio_ctx *ctx,int page_nr)__must_hold (local) +static void bm_page_io_async (struct drbd_bm_aio_ctx *ctx,int page_nr) {struct drbd_device *device = ctx->device;enum req_op op = ctx->flags &BM_AIO_READ ?REQ_OP_READ :REQ_OP_WRITE ;
@@ -1060,7 +1060,8 @@ static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_ho/* * bm_rw: read/write the whole bitmap from/to its on disk location. */ -static int bm_rw (struct drbd_device *device,const unsigned int flags,unsigned lazy_writeout_upper_idx)__must_hold (local) +static int bm_rw (struct drbd_device *device,const unsigned int flags, +unsigned lazy_writeout_upper_idx) {struct drbd_bm_aio_ctx *ctx;struct drbd_bitmap *b = device->bitmap;
@@ -1215,7 +1216,7 @@ static int bm_rw(struct drbd_device *device, const unsigned int flags, unsigned* @device: DRBD device. */ int drbd_bm_read (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {return bm_rw (device,BM_AIO_READ ,0 );
@@ -1228,7 +1229,7 @@ int drbd_bm_read(struct drbd_device *device,* Will only write pages that have changed since last IO. */ int drbd_bm_write (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {return bm_rw (device,0 ,0 ); }
@@ -1240,7 +1241,7 @@ int drbd_bm_write(struct drbd_device *device,* Will write all pages. */ int drbd_bm_write_all (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {return bm_rw (device,BM_AIO_WRITE_ALL_PAGES ,0 ); }
@@ -1250,7 +1251,7 @@ int drbd_bm_write_all(struct drbd_device *device,* @device: DRBD device. * @upper_idx: 0: write all changed pages; +ve: page index to stop scanning for changed pages */ -int drbd_bm_write_lazy (struct drbd_device *device,unsigned upper_idx)__must_hold (local) +int drbd_bm_write_lazy (struct drbd_device *device,unsigned upper_idx) {return bm_rw (device,BM_AIO_COPY_PAGES , upper_idx); }
@@ -1267,7 +1268,7 @@ int drbd_bm_write_lazy(struct drbd_device *device, unsigned upper_idx) __must_ho* pending resync acks are still being processed. */ int drbd_bm_write_copy_pages (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {return bm_rw (device,BM_AIO_COPY_PAGES ,0 ); }
@@ -1276,7 +1277,7 @@ int drbd_bm_write_copy_pages(struct drbd_device *device,* drbd_bm_write_hinted() - Write bitmap pages with "hint" marks, if they have changed. * @device: DRBD device. */ -int drbd_bm_write_hinted (struct drbd_device *device)__must_hold (local) +int drbd_bm_write_hinted (struct drbd_device *device) {return bm_rw (device,BM_AIO_WRITE_HINTED |BM_AIO_COPY_PAGES ,0 ); }
drivers/block/drbd/drbd_int.h +19 -31
97 lines not on this page · raw message
drivers/block/drbd/drbd_main.c +10 -10
@@ -3263,7 +3263,7 @@ void drbd_md_mark_dirty(struct drbd_device *device)mod_timer (&device->md_sync_timer, jiffies +5 *HZ ); } -void drbd_uuid_move_history (struct drbd_device *device)__must_hold (local) +void drbd_uuid_move_history (struct drbd_device *device) {int i;
@@ -3271,7 +3271,7 @@ void drbd_uuid_move_history(struct drbd_device *device) __must_hold(local) device->ldev->md.uuid[i+1 ] = device->ldev->md.uuid[i]; } -void __drbd_uuid_set (struct drbd_device *device,int idx,u64 val)__must_hold (local) +void __drbd_uuid_set (struct drbd_device *device,int idx,u64 val) {if (idx ==UI_CURRENT ) {if (device->state.role ==R_PRIMARY )
@@ -3286,7 +3286,7 @@ void __drbd_uuid_set(struct drbd_device *device, int idx, u64 val) __must_hold(ldrbd_md_mark_dirty (device); } -void _drbd_uuid_set (struct drbd_device *device,int idx,u64 val)__must_hold (local) +void _drbd_uuid_set (struct drbd_device *device,int idx,u64 val) {unsigned long flags;spin_lock_irqsave (&device->ldev->md.uuid_lock, flags);
@@ -3294,7 +3294,7 @@ void _drbd_uuid_set(struct drbd_device *device, int idx, u64 val) __must_hold(lospin_unlock_irqrestore (&device->ldev->md.uuid_lock, flags); } -void drbd_uuid_set (struct drbd_device *device,int idx,u64 val)__must_hold (local) +void drbd_uuid_set (struct drbd_device *device,int idx,u64 val) {unsigned long flags;spin_lock_irqsave (&device->ldev->md.uuid_lock, flags);
@@ -3313,7 +3313,7 @@ void drbd_uuid_set(struct drbd_device *device, int idx, u64 val) __must_hold(loc* Creates a new current UUID, and rotates the old current UUID into * the bitmap slot. Causes an incremental resync upon next connect. */ -void drbd_uuid_new_current (struct drbd_device *device)__must_hold (local) +void drbd_uuid_new_current (struct drbd_device *device) {u64 val;unsigned long long bm_uuid;
@@ -3335,7 +3335,7 @@ void drbd_uuid_new_current(struct drbd_device *device) __must_hold(local)drbd_md_sync (device); } -void drbd_uuid_set_bm (struct drbd_device *device,u64 val)__must_hold (local) +void drbd_uuid_set_bm (struct drbd_device *device,u64 val) {unsigned long flags;spin_lock_irqsave (&device->ldev->md.uuid_lock, flags);
@@ -3368,7 +3368,7 @@ void drbd_uuid_set_bm(struct drbd_device *device, u64 val) __must_hold(local)* Sets all bits in the bitmap and writes the whole bitmap to stable storage. */ int drbd_bmio_set_n_write (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {int rv = -EIO ;
@@ -3395,7 +3395,7 @@ int drbd_bmio_set_n_write(struct drbd_device *device,* Clears all bits in the bitmap and writes the whole bitmap to stable storage. */ int drbd_bmio_clear_n_write (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {drbd_resume_al (device);
@@ -3522,7 +3522,7 @@ int drbd_bitmap_io(struct drbd_device *device,return rv; } -void drbd_md_set_flag (struct drbd_device *device,int flag)__must_hold (local) +void drbd_md_set_flag (struct drbd_device *device,int flag) {if ((device->ldev->md.flags & flag) != flag) {drbd_md_mark_dirty (device);
@@ -3530,7 +3530,7 @@ void drbd_md_set_flag(struct drbd_device *device, int flag) __must_hold(local) } } -void drbd_md_clear_flag (struct drbd_device *device,int flag)__must_hold (local) +void drbd_md_clear_flag (struct drbd_device *device,int flag) {if ((device->ldev->md.flags & flag) !=0 ) {drbd_md_mark_dirty (device);
drivers/block/drbd/drbd_nl.c +4 -3
@@ -968,8 +968,9 @@ void drbd_resume_io(struct drbd_device *device)* Returns 0 on success, negative return values indicate errors. * You should call drbd_md_sync() after calling this function. */ -enum determine_dev_size -drbd_determine_dev_size (struct drbd_device *device,enum dds_flags flags,struct resize_parms *rs)__must_hold (local) +enum determine_dev_size drbd_determine_dev_size (struct drbd_device *device, +enum dds_flags flags, +struct resize_parms *rs) {struct md_offsets_and_sizes {u64 last_agreed_sect;
@@ -3097,7 +3098,7 @@ static int drbd_adm_simple_request_state(struct sk_buff *skb, struct genl_info * }static int drbd_bmio_set_susp_al (struct drbd_device *device, -struct drbd_peer_device *peer_device)__must_hold (local) +struct drbd_peer_device *peer_device) {int rv;
drivers/block/drbd/drbd_receiver.c +11 -10
@@ -174,8 +174,9 @@ You must not have the req_lock:* w_same: payload_size == logical_block_size * trim: payload_size == 0 */ struct drbd_peer_request * -drbd_alloc_peer_req (struct drbd_peer_device *peer_device,u64 id,sector_t sector, -unsigned int request_size,unsigned int payload_size,gfp_t gfp_mask)__must_hold (local) +drbd_alloc_peer_req (struct drbd_peer_device *peer_device,u64 id, +sector_t sector,unsigned int request_size, +unsigned int payload_size,gfp_t gfp_mask) {struct drbd_device *device = peer_device->device;struct drbd_peer_request *peer_req;
@@ -1657,7 +1658,7 @@ static void drbd_csum_ee_size(struct crypto_shash *h,*/ static struct drbd_peer_request *read_in_block (struct drbd_peer_device *peer_device,u64 id,sector_t sector, -struct packet_info *pi)__must_hold (local) +struct packet_info *pi) {struct drbd_device *device = peer_device->device;const sector_t capacity =get_capacity (device->vdisk);
@@ -1868,8 +1869,8 @@ static int e_end_resync_block(struct drbd_work *w, int unused)return err; } -static int recv_resync_read (struct drbd_peer_device *peer_device,sector_t sector, -struct packet_info *pi)__releases (local) +static int recv_resync_read (struct drbd_peer_device *peer_device, +sector_t sector,struct packet_info *pi) {struct drbd_device *device = peer_device->device;struct drbd_peer_request *peer_req;
@@ -2826,7 +2827,7 @@ static int receive_DataRequest(struct drbd_connection *connection, struct packet/* * drbd_asb_recover_0p - Recover after split-brain with no remaining primaries */ -static int drbd_asb_recover_0p (struct drbd_peer_device *peer_device)__must_hold (local) +static int drbd_asb_recover_0p (struct drbd_peer_device *peer_device) {struct drbd_device *device = peer_device->device;int self, peer, rv =-100 ;
@@ -2909,7 +2910,7 @@ static int drbd_asb_recover_0p(struct drbd_peer_device *peer_device) __must_hold/* * drbd_asb_recover_1p - Recover after split-brain with one remaining primary */ -static int drbd_asb_recover_1p (struct drbd_peer_device *peer_device)__must_hold (local) +static int drbd_asb_recover_1p (struct drbd_peer_device *peer_device) {struct drbd_device *device = peer_device->device;int hg, rv =-100 ;
@@ -2966,7 +2967,7 @@ static int drbd_asb_recover_1p(struct drbd_peer_device *peer_device) __must_hold/* * drbd_asb_recover_2p - Recover after split-brain with two remaining primaries */ -static int drbd_asb_recover_2p (struct drbd_peer_device *peer_device)__must_hold (local) +static int drbd_asb_recover_2p (struct drbd_peer_device *peer_device) {struct drbd_device *device = peer_device->device;int hg, rv =-100 ;
@@ -3044,7 +3045,7 @@ static void drbd_uuid_dump(struct drbd_device *device, char *text, u64 *uuid,*/ static int drbd_uuid_compare (struct drbd_peer_device *const peer_device, -enum drbd_role const peer_role,int *rule_nr)__must_hold (local) +enum drbd_role const peer_role,int *rule_nr) {struct drbd_connection *const connection = peer_device->connection;struct drbd_device *device = peer_device->device;
@@ -3264,7 +3265,7 @@ static int drbd_uuid_compare(struct drbd_peer_device *const peer_device,*/ static enum drbd_conns drbd_sync_handshake (struct drbd_peer_device *peer_device,enum drbd_role peer_role, -enum drbd_disk_state peer_disk)__must_hold (local) +enum drbd_disk_state peer_disk) {struct drbd_device *device = peer_device->device;enum drbd_conns rv =C_MASK ;
drivers/block/drbd/drbd_worker.c +2 -4
@@ -78,7 +78,7 @@ void drbd_md_endio(struct bio *bio)/* reads on behalf of the partner, * "submitted" by the receiver */ -static void drbd_endio_read_sec_final (struct drbd_peer_request *peer_req)__releases (local) +static void drbd_endio_read_sec_final (struct drbd_peer_request *peer_req) {unsigned long flags =0 ;struct drbd_peer_device *peer_device = peer_req->peer_device;
@@ -99,7 +99,7 @@ static void drbd_endio_read_sec_final(struct drbd_peer_request *peer_req) __rele/* writes on behalf of the partner, or resync writes, * "submitted" by the receiver, final stage. */ -void drbd_endio_write_sec_final (struct drbd_peer_request *peer_req)__releases (local) +void drbd_endio_write_sec_final (struct drbd_peer_request *peer_req) {unsigned long flags =0 ;struct drbd_peer_device *peer_device = peer_req->peer_device;
@@ -1923,10 +1923,8 @@ static void drbd_ldev_destroy(struct drbd_device *device)lc_destroy (device->act_log); device->act_log =NULL ; -__acquire (local);drbd_backing_dev_free (device, device->ldev); device->ldev =NULL ; -__release (local);clear_bit (GOING_DISKLESS , &device->flags);wake_up (&device->misc_wait);
Re: [PATCH 05/27] drbd: Remove the 'local' lock context
Quoted 4 lines · Bart Van Assche
The 'local' lock context represents code sections between
get_ldev()andput_ldev()calls. Between these two calls thedevice->ldevpointer is valid. Since there are multiple functions with unpairedget_ldev()/put_ldev()calls, remove this lock context.
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 6/27 drbd: Simplify the bitmap locking functions.
In reply to Bart Van AsscheCall mutex_lock() instead of mutex_trylock() followed by mutex_lock(). Remove the code that depends on device->bitmap == NULL because this can't happen. All drbd_bm_{lock,unlock}() callers guarantee that the device->bitmap pointer is valid.
Suggested-by Christoph Hellwig <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_bitmap.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
drivers/block/drbd/drbd_bitmap.c +1 -19
@@ -124,22 +124,8 @@ static void __bm_print_lock_info(struct drbd_device *device, const char *func)void drbd_bm_lock (struct drbd_device *device,char *why,enum bm_flag flags) {struct drbd_bitmap *b = device->bitmap; -int trylock_failed; -if (!b) { -drbd_err (device,"FIXME no bitmap in drbd_bm_lock!?\n" ); -return ; - } - - trylock_failed = !mutex_trylock (&b->bm_change); - -if (trylock_failed) { -drbd_warn (device,"%s[%d] going to '%s' but bitmap already locked for '%s' by %s[%d]\n" , - current->comm,task_pid_nr (current), - why, b->bm_why ?:"?" , - b->bm_task->comm,task_pid_nr (b->bm_task)); -mutex_lock (&b->bm_change); - } +mutex_lock (&b->bm_change);if (BM_LOCKED_MASK & b->bm_flags)drbd_err (device,"FIXME bitmap already locked in bm_lock\n" ); b->bm_flags |= flags &BM_LOCKED_MASK ;
@@ -151,10 +137,6 @@ void drbd_bm_lock(struct drbd_device *device, char *why, enum bm_flag flags)void drbd_bm_unlock (struct drbd_device *device) {struct drbd_bitmap *b = device->bitmap; -if (!b) { -drbd_err (device,"FIXME no bitmap in drbd_bm_unlock!?\n" ); -return ; - }if (!(BM_LOCKED_MASK & device->bitmap->bm_flags))drbd_err (device,"FIXME bitmap not locked in bm_unlock\n" );
Re: [PATCH 06/27] drbd: Simplify the bitmap locking functions.
Quoted 4 lines · Bart Van Assche
Call
mutex_lock()instead ofmutex_trylock()followed bymutex_lock(). Remove the code that depends ondevice->bitmap== NULL because this can't happen. All drbd_bm_{lock,unlock}() callers guarantee that thedevice->bitmappointer is valid.
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 7/27 drbd: Move two declarations
In reply to Bart Van AsscheMove the resources_mutex declaration in front of the functions that acquire and release this mutex. Move the drbd_wait_misc() declaration past the struct drbd_device definition. This patch prepares for adding lock context annotations.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_int.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/block/drbd/drbd_int.h +2 -3
@@ -195,7 +195,7 @@ struct drbd_device_work {#include "drbd_interval.h" -int drbd_wait_misc (struct drbd_device *,struct drbd_interval *); +extern struct mutex resources_mutex;void lock_all_resources (void );void unlock_all_resources (void );
@@ -1094,6 +1094,7 @@ int drbd_bmio_set_n_write(struct drbd_device *device, struct drbd_peer_device *peer_device);int drbd_bmio_clear_n_write (struct drbd_device *device,struct drbd_peer_device *peer_device); +int drbd_wait_misc (struct drbd_device *device,struct drbd_interval *i);/* Meta data layout *
@@ -1363,8 +1364,6 @@ extern struct bio_set drbd_md_io_bio_set;/* And a bio_set for cloning */ extern struct bio_set drbd_io_bio_set; -extern struct mutex resources_mutex; -enum drbd_ret_code drbd_create_device (struct drbd_config_context *adm_ctx,unsigned int minor);void drbd_destroy_device (struct kref *kref);
Re: [PATCH 07/27] drbd: Move two declarations
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 8/27 drbd: Pass 'resource' directly to complete_conflicting_writes()
In reply to Bart Van AsschePrepare for adding lock context annotations. Without this change, an __assume_ctx_lock() statement would have to be added.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_req.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/block/drbd/drbd_req.c +5 -4
@@ -951,7 +951,8 @@ static bool remote_due_to_read_balancing(struct drbd_device *device, sector_t se* * Only way out: remove the conflicting intervals from the tree. */ -static void complete_conflicting_writes (struct drbd_request *req) +static void complete_conflicting_writes (struct drbd_resource *resource, +struct drbd_request *req) {DEFINE_WAIT (wait);struct drbd_device *device = req->device;
@@ -974,9 +975,9 @@ static void complete_conflicting_writes(struct drbd_request *req)/* Indicate to wake up device->misc_wait on progress. */ prepare_to_wait (&device->misc_wait, &wait,TASK_UNINTERRUPTIBLE ); i->waiting = true; -spin_unlock_irq (&device->resource->req_lock); +spin_unlock_irq (&resource->req_lock);schedule (); -spin_lock_irq (&device->resource->req_lock); +spin_lock_irq (&resource->req_lock); }finish_wait (&device->misc_wait, &wait); }
@@ -1329,7 +1330,7 @@ static void drbd_send_and_submit(struct drbd_device *device, struct drbd_request/* This may temporarily give up the req_lock, * but will re-aquire it before it returns here. * Needs to be before the check on drbd_suspended() */ -complete_conflicting_writes (req); +complete_conflicting_writes (resource, req);/* no more giving up req_lock from now on! */ /* check for congestion, and potentially stop sending
Re: [PATCH 08/27] drbd: Pass 'resource' directly to complete_conflicting_writes()
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 9/27 drbd: Split drbd_nl_get_connections_dumpit()
In reply to Bart Van AsscheMove the code between the 'put_result' and 'out' labels into a new function. Prepare for making the following code unconditional:
if (resource) mutex_unlock(&resource->conf_update);
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_nl.c | 98 ++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 43 deletions(-)
drivers/block/drbd/drbd_nl.c +55 -43
@@ -3529,15 +3529,56 @@ int drbd_adm_dump_connections_done(struct netlink_callback *cb)enum {SINGLE_RESOURCE ,ITERATE_RESOURCES }; +static int drbd_nl_put_dump_connections_result ( +struct sk_buff *skb,struct netlink_callback *cb, +struct drbd_resource *resource,struct drbd_connection *connection, +enum drbd_ret_code retcode) +{ +struct drbd_genlmsghdr *dh; + + dh =genlmsg_put (skb,NETLINK_CB (cb->skb).portid, cb->nlh->nlmsg_seq, + &drbd_nl_family,NLM_F_MULTI , +DRBD_ADM_GET_CONNECTIONS ); +if (!dh) +return -ENOMEM ; + dh->ret_code = retcode; + dh->minor =-1U ; +if (retcode ==NO_ERROR ) { +struct connection_statistics connection_statistics; +struct connection_info connection_info; +struct net_conf *net_conf; +int err; + + err =nla_put_drbd_cfg_context (skb, resource, connection,NULL ); +if (err) +return err; + net_conf =rcu_dereference (connection->net_conf); +if (net_conf) { + err =net_conf_to_skb (skb, net_conf); +if (err) +return err; + } +connection_to_info (&connection_info, connection); + err =connection_info_to_skb (skb, &connection_info); +if (err) +return err; + connection_statistics.conn_congested = +test_bit (NET_CONGESTED , &connection->flags); + err =connection_statistics_to_skb (skb, &connection_statistics); +if (err) +return err; + cb->args[2 ] = (long )connection; + } +genlmsg_end (skb, dh); +return 0 ; +} +int drbd_nl_get_connections_dumpit (struct sk_buff *skb,struct netlink_callback *cb) {struct nlattr *resource_filter;struct drbd_resource *resource =NULL , *next_resource;struct drbd_connection *connection; -int err =0 , retcode; -struct drbd_genlmsghdr *dh; -struct connection_info connection_info; -struct connection_statistics connection_statistics; +int err =0 ;rcu_read_lock (); resource = (struct drbd_resource *)cb->args[0 ];
@@ -3545,10 +3586,13 @@ int drbd_nl_get_connections_dumpit(struct sk_buff *skb, struct netlink_callback resource_filter =find_cfg_context_attr (cb->nlh,DRBD_A_DRBD_CFG_CONTEXT_CTX_RESOURCE_NAME );if (resource_filter) { - retcode =ERR_RES_NOT_KNOWN ; resource =drbd_find_resource (nla_data (resource_filter)); -if (!resource) - goto put_result; +if (!resource) { + err =drbd_nl_put_dump_connections_result ( + skb, cb, resource,NULL , +ERR_RES_NOT_KNOWN ); + goto out; + } cb->args[0 ] = (long )resource; cb->args[1 ] =SINGLE_RESOURCE ; }
@@ -3579,8 +3623,9 @@ int drbd_nl_get_connections_dumpit(struct sk_buff *skb, struct netlink_callbacklist_for_each_entry_continue_rcu (connection, &resource->connections, connections) {if (!has_net_conf (connection))continue ; - retcode =NO_ERROR ; - goto put_result;/* only one iteration */ + err =drbd_nl_put_dump_connections_result (skb, cb, resource, + connection,NO_ERROR ); + goto out;/* only one iteration */ } no_more_connections:
@@ -3603,41 +3648,8 @@ int drbd_nl_get_connections_dumpit(struct sk_buff *skb, struct netlink_callback cb->args[2 ] =0 ; goto next_resource; } - goto out;/* no more resources */ - -put_result: - dh =genlmsg_put (skb,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, &drbd_nl_family, -NLM_F_MULTI ,DRBD_ADM_GET_CONNECTIONS ); - err = -ENOMEM ; -if (!dh) - goto out; - dh->ret_code = retcode; - dh->minor =-1U ; -if (retcode ==NO_ERROR ) { -struct net_conf *net_conf; - err =nla_put_drbd_cfg_context (skb, resource, connection,NULL ); -if (err) - goto out; - net_conf =rcu_dereference (connection->net_conf); -if (net_conf) { - err =net_conf_to_skb (skb, net_conf); -if (err) - goto out; - } -connection_to_info (&connection_info, connection); - err =connection_info_to_skb (skb, &connection_info); -if (err) - goto out; - connection_statistics.conn_congested =test_bit (NET_CONGESTED , &connection->flags); - err =connection_statistics_to_skb (skb, &connection_statistics); -if (err) - goto out; - cb->args[2 ] = (long )connection; - } -genlmsg_end (skb, dh); - err =0 ; +/* no more resources */ out:rcu_read_unlock ();
Re: [PATCH 09/27] drbd: Split drbd_nl_get_connections_dumpit()
Quoted 16 lines · Bart Van Assche
Move the code between the '
put_result' and 'out' labels into a new function. Prepare for making the following code unconditional:if (resource) mutex_unlock(&resource->conf_update);Signed-off-by Bart Van Assche <[email protected]>--- drivers/block/drbd/drbd_nl.c | 98 ++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 43 deletions(-)drivers/block/drbd/drbd_nl.c +0
-0
drivers/block/drbd/drbd_nl.c int drbd_adm_dump_connections_done(struct netlink_callback *cb) · line 3535
Quoted 7 lines
drivers/block/drbd/drbd_nl.c +4
-0@@ -3529,15 +3529,56 @@ int drbd_adm_dump_connections_done(struct netlink_callback *cb)enum {SINGLE_RESOURCE ,ITERATE_RESOURCES }; +static int drbd_nl_put_dump_connections_result ( +struct sk_buff *skb,struct netlink_callback *cb, +struct drbd_resource *resource,struct drbd_connection *connection, +enum drbd_ret_code retcode)
Weird indenation here. The usual style is either lining up after the opening brace or two tabs.
Re: [PATCH 09/27] drbd: Split drbd_nl_get_connections_dumpit()
Quoted 8 lines · Christoph Hellwig
drivers/block/drbd/drbd_nl.c int drbd_adm_dump_connections_done(struct netlink_callback *cb) · line 3535
Quoted 4 lines · Bart Van Assche
drivers/block/drbd/drbd_nl.c +4
-0+static int drbd_nl_put_dump_connections_result ( +struct sk_buff *skb,struct netlink_callback *cb, +struct drbd_resource *resource,struct drbd_connection *connection, +enum drbd_ret_code retcode)Weird indenation here. The usual style is either lining up after the opening brace or two tabs.
This formatting comes from git clang-format. Anyway, I will fix the formatting.
Thanks,
Bart.
PATCH 10/27 drbd: Make a mutex_unlock() call unconditional
In reply to Bart Van AsscheOnly jump to the 'out' label if resource is not NULL such that the mutex_unlock() call past the 'out' label can be made unconditional.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_nl.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/block/drbd/drbd_nl.c +9 -5
@@ -3591,15 +3591,18 @@ int drbd_nl_get_connections_dumpit(struct sk_buff *skb, struct netlink_callback err =drbd_nl_put_dump_connections_result ( skb, cb, resource,NULL ,ERR_RES_NOT_KNOWN ); - goto out; +rcu_read_unlock (); + goto ret; } cb->args[0 ] = (long )resource; cb->args[1 ] =SINGLE_RESOURCE ; } }if (!resource) { -if (list_empty (&drbd_resources)) - goto out; +if (list_empty (&drbd_resources)) { +rcu_read_unlock (); + goto ret; + } resource = list_first_entry(&drbd_resources, struct drbd_resource, resources);kref_get (&resource->kref); cb->args[0 ] = (long )resource;
@@ -3653,8 +3656,9 @@ int drbd_nl_get_connections_dumpit(struct sk_buff *skb, struct netlink_callback out:rcu_read_unlock (); -if (resource) -mutex_unlock (&resource->conf_update); +mutex_unlock (&resource->conf_update); + +ret:if (err)return err;return skb->len;
Re: [PATCH 10/27] drbd: Make a mutex_unlock() call unconditional
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 11/27 drbd: Split drbd_req_state()
In reply to Bart Van AsscheMove all code except a mutex_lock() and a mutex_unlock() call from drbd_req_state() into the new function __drbd_req_state(). This patch prepares for adding a second __drbd_req_state() caller.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_state.c | 43 +++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 15 deletions(-)
drivers/block/drbd/drbd_state.c +28 -15
@@ -549,19 +549,10 @@ _req_st_cond(struct drbd_device *device, union drbd_state mask,return rv; } -/** -* drbd_req_state() - Perform an eventually cluster wide state change -* @device: DRBD device. -* @mask: mask of state bits to change. -* @val: value of new state bits. -* @f: flags -* -* Should not be called directly, use drbd_request_state() or -* _drbd_request_state(). -*/ -static enum drbd_state_rv -drbd_req_state (struct drbd_device *device,union drbd_state mask, -union drbd_state val,enum chg_state_flags f) +static enum drbd_state_rv __drbd_req_state (struct drbd_device *device, +union drbd_state mask, +union drbd_state val, +enum chg_state_flags f) {struct completion done;unsigned long flags;
@@ -571,8 +562,6 @@ drbd_req_state(struct drbd_device *device, union drbd_state mask,init_completion (&done); -if (f &CS_SERIALIZE ) -mutex_lock (device->state_mutex);if (f &CS_INHIBIT_MD_IO ) buffer =drbd_md_get_buffer (device, __func__);
@@ -629,6 +618,30 @@ drbd_req_state(struct drbd_device *device, union drbd_state mask, abort:if (buffer)drbd_md_put_buffer (device); + +return rv; +} + +/** +* drbd_req_state() - Perform an eventually cluster wide state change +* @device: DRBD device. +* @mask: mask of state bits to change. +* @val: value of new state bits. +* @f: flags +* +* Should not be called directly, use drbd_request_state() or +* _drbd_request_state(). +*/ +static enum drbd_state_rv drbd_req_state (struct drbd_device *device, +union drbd_state mask, +union drbd_state val, +enum chg_state_flags f) +{ +enum drbd_state_rv rv; + +if (f &CS_SERIALIZE ) +mutex_lock (device->state_mutex); + rv =__drbd_req_state (device, mask, val, f & ~CS_SERIALIZE );if (f &CS_SERIALIZE )mutex_unlock (device->state_mutex);
Re: [PATCH 11/27] drbd: Split drbd_req_state()
drivers/block/drbd/drbd_state.c drbd_req_state(struct drbd_device *device, union drbd_state mask, · line 646
Quoted 8 lines
drivers/block/drbd/drbd_state.c +6
-0+{ +enum drbd_state_rv rv; + +if (f &CS_SERIALIZE ) +mutex_lock (device->state_mutex); + rv =__drbd_req_state (device, mask, val, f & ~CS_SERIALIZE );if (f &CS_SERIALIZE )mutex_unlock (device->state_mutex);
Wouldn't something like:
if (f & CS_SERIALIZE) {
mutex_lock(device->state_mutex);
rv = __drbd_req_state(device, mask, val, f & ~CS_SERIALIZE);
mutex_unlock(device->state_mutex);
} else {
rv = __drbd_req_state(device, mask, val, f & ~CS_SERIALIZE);
}be either to follow? Is there much of a point in the CS_SERIALIZE clearing here?
Re: [PATCH 11/27] drbd: Split drbd_req_state()
Quoted 19 lines · Christoph Hellwig
drivers/block/drbd/drbd_state.c drbd_req_state(struct drbd_device *device, union drbd_state mask, · line 646
Quoted 8 lines
drivers/block/drbd/drbd_state.c +6
-0+{ +enum drbd_state_rv rv; + +if (f &CS_SERIALIZE ) +mutex_lock (device->state_mutex); + rv =__drbd_req_state (device, mask, val, f & ~CS_SERIALIZE );if (f &CS_SERIALIZE )mutex_unlock (device->state_mutex);Wouldn't something like:
if (f & CS_SERIALIZE) { mutex_lock(device->state_mutex); rv = __drbd_req_state(device, mask, val, f & ~CS_SERIALIZE); mutex_unlock(device->state_mutex); } else { rv = __drbd_req_state(device, mask, val, f & ~CS_SERIALIZE); }
Quoted 2 lines
be either to follow? Is there much of a point in the
CS_SERIALIZEclearing here?
Do you perhaps want me to combine this patch and the next patch in this series (12/27)?
Thanks,
Bart.
Re: [PATCH 11/27] drbd: Split drbd_req_state()
Quoted 2 lines · Bart Van Assche
Do you perhaps want me to combine this patch and the next patch in this series (12/27)?
Answering my own question: I just noticed that this has been requested in the review comment on the next patch. I will combine these two patches.
Bart.
PATCH 12/27 drbd: Convert drbd_req_state() to unconditional locking
In reply to Bart Van AsschePrepare for enabling lock context analysis.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_state.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/block/drbd/drbd_state.c +5 -4
@@ -639,11 +639,12 @@ static enum drbd_state_rv drbd_req_state(struct drbd_device *device, {enum drbd_state_rv rv; -if (f &CS_SERIALIZE ) -mutex_lock (device->state_mutex); +if (!(f &CS_SERIALIZE )) +return __drbd_req_state (device, mask, val, f); + +mutex_lock (device->state_mutex); rv =__drbd_req_state (device, mask, val, f & ~CS_SERIALIZE ); -if (f &CS_SERIALIZE ) -mutex_unlock (device->state_mutex); +mutex_unlock (device->state_mutex);return rv; }
Re: [PATCH 12/27] drbd: Convert drbd_req_state() to unconditional locking
Quoted 15 lines · Bart Van Assche
Prepare for enabling lock context analysis.
Signed-off-by Bart Van Assche <[email protected]>--- drivers/block/drbd/drbd_state.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)drivers/block/drbd/drbd_state.c +0
-0@@ -639,11 +639,12 @@ static enum drbd_state_rv drbd_req_state(struct drbd_device *device, {enum drbd_state_rv rv;
drivers/block/drbd/drbd_state.c static enum drbd_state_rv drbd_req_state(struct drbd_device *device, · line 647
Quoted 10 lines
drivers/block/drbd/drbd_state.c +5
-4-if (f &CS_SERIALIZE ) -mutex_lock (device->state_mutex); +if (!(f &CS_SERIALIZE )) +return __drbd_req_state (device, mask, val, f); + +mutex_lock (device->state_mutex); rv =__drbd_req_state (device, mask, val, f & ~CS_SERIALIZE ); -if (f &CS_SERIALIZE ) -mutex_unlock (device->state_mutex); +mutex_unlock (device->state_mutex);
It's not really unconditional, just different conditions. And really belongs into the previous patch.
PATCH 13/27 drbd: Annotate drbd_bm_{lock,unlock}()
In reply to Bart Van AsscheAnnotate the bitmap locking functions. Introduce a new token context lock because the struct drbd_device definition is not visible from the drbd_int.h header file. Annotate the declarations of bitmap locking functions with only the token context lock. Annotate the definitions of the bitmap locking functions with both the token context lock and the actual mutex.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/drbd_bitmap.c | 6 ++++++ drivers/block/drbd/drbd_int.h | 8 ++++++-- 2 files changed, 12 insertions(+), 2 deletions(-)
drivers/block/drbd/drbd_bitmap.c +6 -0
@@ -122,9 +122,12 @@ static void __bm_print_lock_info(struct drbd_device *device, const char *func) }void drbd_bm_lock (struct drbd_device *device,char *why,enum bm_flag flags) +__acquires (drbd_dev_bm_lock) + __acquires(&device->bitmap->bm_change) {struct drbd_bitmap *b = device->bitmap; +__acquire (drbd_dev_bm_lock);mutex_lock (&b->bm_change);if (BM_LOCKED_MASK & b->bm_flags)drbd_err (device,"FIXME bitmap already locked in bm_lock\n" );
@@ -135,6 +138,8 @@ void drbd_bm_lock(struct drbd_device *device, char *why, enum bm_flag flags) }void drbd_bm_unlock (struct drbd_device *device) +__releases (drbd_dev_bm_lock) + __releases(&device->bitmap->bm_change) {struct drbd_bitmap *b = device->bitmap;
@@ -145,6 +150,7 @@ void drbd_bm_unlock(struct drbd_device *device) b->bm_why =NULL ; b->bm_task =NULL ;mutex_unlock (&b->bm_change); +__release (drbd_dev_bm_lock); }/* we store some "meta" info about our pages in page->private */
drivers/block/drbd/drbd_int.h +6 -2
@@ -1337,8 +1337,12 @@ void drbd_bm_merge_lel(struct drbd_device *device, size_t offset, size_t number,void drbd_bm_get_lel (struct drbd_device *device,size_t offset,size_t number,unsigned long *buffer); -void drbd_bm_lock (struct drbd_device *device,char *why,enum bm_flag flags); -void drbd_bm_unlock (struct drbd_device *device); +token_context_lock (drbd_dev_bm_lock); + +void drbd_bm_lock(struct drbd_device *device, char *why, enum bm_flag flags) + __acquires(drbd_dev_bm_lock); +void drbd_bm_unlock(struct drbd_device *device) + __releases(drbd_dev_bm_lock);/* drbd_main.c */ extern struct kmem_cache *drbd_request_cache;
Re: [PATCH 13/27] drbd: Annotate drbd_bm_{lock,unlock}()
Looks good:
Reviewed-by Christoph Hellwig <[email protected]>
PATCH 14/27 drbd: Enable lock context analysis
In reply to Bart Van AsscheClang performs more strict checking of lock context annotations than sparse. This patch makes the DRBD lock context annotations compatible with Clang and enables lock context analysis in the Makefile.
Cc Christoph Böhmwalder <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/drbd/Makefile | 3 +++ drivers/block/drbd/drbd_int.h | 27 ++++++++++++++++++--------- drivers/block/drbd/drbd_main.c | 2 ++ drivers/block/drbd/drbd_receiver.c | 6 ++++++ drivers/block/drbd/drbd_req.c | 1 + drivers/block/drbd/drbd_state.c | 2 ++ drivers/block/drbd/drbd_state.h | 4 ++-- 7 files changed, 34 insertions(+), 11 deletions(-)
drivers/block/drbd/Makefile +3 -0
@@ -1,4 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only + +CONTEXT_ANALYSIS := y + drbd-y := drbd_buildtag.o drbd_bitmap.o drbd_proc.o drbd-y += drbd_worker.o drbd_receiver.o drbd_req.o drbd_actlog.o drbd-y += drbd_main.o drbd_strings.o drbd_nl.o
drivers/block/drbd/drbd_int.h +18 -9
@@ -197,8 +197,8 @@ struct drbd_device_work {extern struct mutex resources_mutex; -void lock_all_resources (void ); -void unlock_all_resources (void ); +void lock_all_resources(void) __acquires(&resources_mutex); +void unlock_all_resources(void) __releases(&resources_mutex);struct drbd_request {struct drbd_work w;
@@ -1094,7 +1094,8 @@ int drbd_bmio_set_n_write(struct drbd_device *device, struct drbd_peer_device *peer_device);int drbd_bmio_clear_n_write (struct drbd_device *device,struct drbd_peer_device *peer_device); -int drbd_wait_misc (struct drbd_device *device,struct drbd_interval *i); +int drbd_wait_misc(struct drbd_device *device, struct drbd_interval *i) + __must_hold(&device->resource->req_lock);/* Meta data layout *
@@ -1859,12 +1860,20 @@ static inline void request_ping(struct drbd_connection *connection)wake_ack_receiver (connection); } -void *conn_prepare_command (struct drbd_connection *,struct drbd_socket *); -void *drbd_prepare_command (struct drbd_peer_device *,struct drbd_socket *); -int conn_send_command (struct drbd_connection *,struct drbd_socket *, -enum drbd_packet ,unsigned int ,void *,unsigned int ); -int drbd_send_command (struct drbd_peer_device *,struct drbd_socket *, -enum drbd_packet ,unsigned int ,void *,unsigned int ); +void *conn_prepare_command (struct drbd_connection *connection, +struct drbd_socket *sock) + __cond_acquires(true, &sock->mutex); +void *drbd_prepare_command (struct drbd_peer_device *peer_device,struct drbd_socket *sock) + __cond_acquires(true, &sock->mutex); +int conn_send_command(struct drbd_connection *connection, + struct drbd_socket *sock, + enum drbd_packet cmd, unsigned int header_size, + void *data, unsigned int size) + __releases(&sock->mutex); +int drbd_send_command(struct drbd_peer_device *peer_device, struct drbd_socket *sock, + enum drbd_packet cmd, unsigned int header_size, + void *data, unsigned int size) + __releases(&sock->mutex);int drbd_send_ping (struct drbd_connection *connection);int drbd_send_ping_ack (struct drbd_connection *connection);
drivers/block/drbd/drbd_main.c +2 -0
@@ -3659,6 +3659,7 @@ int drbd_wait_misc(struct drbd_device *device, struct drbd_interval *i) }void lock_all_resources (void ) + __context_unsafe(locking loop) {struct drbd_resource *resource;int __maybe_unused i =0 ;
@@ -3670,6 +3671,7 @@ void lock_all_resources(void) }void unlock_all_resources (void ) + __context_unsafe(locking loop) {struct drbd_resource *resource;
drivers/block/drbd/drbd_receiver.c +6 -0
@@ -288,6 +288,7 @@ static int drbd_finish_peer_reqs(struct drbd_device *device)static void _drbd_wait_ee_list_empty (struct drbd_device *device,struct list_head *head) + __must_hold(&device->resource->req_lock) {DEFINE_WAIT (wait);
@@ -897,6 +898,8 @@ static int conn_connect(struct drbd_connection *connection)if (drbd_send_protocol (connection) == -EOPNOTSUPP )return -1 ; +/* context_unsafe() because of two locking loops */ + context_unsafe(/* Prevent a race between resync-handshake and * being promoted to Primary. *
@@ -914,6 +917,7 @@ static int conn_connect(struct drbd_connection *connection)idr_for_each_entry (&connection->peer_devices, peer_device, vnr)mutex_unlock (peer_device->device->state_mutex); + );rcu_read_lock ();idr_for_each_entry (&connection->peer_devices, peer_device, vnr) {
@@ -2231,6 +2235,7 @@ static blk_opf_t wire_flags_to_bio(struct drbd_connection *connection, u32 dpf)static void fail_postponed_requests (struct drbd_device *device,sector_t sector,unsigned int size) + __must_hold(&device->resource->req_lock) {struct drbd_peer_device *peer_device =first_peer_device (device);struct drbd_interval *i;
@@ -2257,6 +2262,7 @@ static void fail_postponed_requests(struct drbd_device *device, sector_t sector,static int handle_write_conflicts (struct drbd_device *device,struct drbd_peer_request *peer_req) + __must_hold(&device->resource->req_lock) {struct drbd_connection *connection = peer_req->peer_device->connection;bool resolve_conflicts =test_bit (RESOLVE_CONFLICTS , &connection->flags);
drivers/block/drbd/drbd_req.c +1 -0
@@ -953,6 +953,7 @@ static bool remote_due_to_read_balancing(struct drbd_device *device, sector_t se*/ static void complete_conflicting_writes (struct drbd_resource *resource,struct drbd_request *req) + __must_hold(&resource->req_lock) {DEFINE_WAIT (wait);struct drbd_device *device = req->device;
drivers/block/drbd/drbd_state.c +2 -0
@@ -713,6 +713,7 @@ int drbd_request_detach_interruptible(struct drbd_device *device)enum drbd_state_rv _drbd_request_state_holding_state_mutex (struct drbd_device *device,union drbd_state mask,union drbd_state val,enum chg_state_flags f) + __must_hold(&device->state_mutex) {enum drbd_state_rv rv;
@@ -2306,6 +2307,7 @@ _conn_rq_cond(struct drbd_connection *connection, union drbd_state mask, union denum drbd_state_rv _conn_request_state (struct drbd_connection *connection,union drbd_state mask,union drbd_state val,enum chg_state_flags flags) + __context_unsafe(conditional locking) {enum drbd_state_rv rv =SS_SUCCESS ;struct after_conn_state_chg_work *acscw;
drivers/block/drbd/drbd_state.h +2 -2
@@ -121,8 +121,8 @@ enum drbd_state_rv _drbd_request_state(struct drbd_device *, union drbd_state, union drbd_state, enum chg_state_flags);enum drbd_state_rv -_drbd_request_state_holding_state_mutex (struct drbd_device *,union drbd_state , -union drbd_state ,enum chg_state_flags ); +_drbd_request_state_holding_state_mutex (struct drbd_device *device,union drbd_state mask, +union drbd_state val,enum chg_state_flags f); enum drbd_state_rv _drbd_set_state(struct drbd_device *, union drbd_state, enum chg_state_flags,
PATCH 15/27 loop: Split loop_change_fd()
In reply to Bart Van AsschePrepare for adding a second call of __loop_change_fd().
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/loop.c | 85 ++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 39 deletions(-)
drivers/block/loop.c +46 -39
@@ -523,6 +523,49 @@ static int loop_check_backing_file(struct file *file)return 0 ; } +static int __loop_change_fd (struct loop_device *lo,struct block_device *bdev, +struct file *file,struct file **old_file, +bool *partscan) +{ +unsigned int memflags; +int error; + +if (lo->lo_state != Lo_bound) +return -ENXIO ; + +/* the loop device has to be read-only */ +if (!(lo->lo_flags &LO_FLAGS_READ_ONLY )) +return -EINVAL ; + + error =loop_validate_file (file, bdev); +if (error) +return error; + + *old_file = lo->lo_backing_file; + +/* size of the new backing store needs to be the same */ +if (lo_calculate_size (lo, file) !=lo_calculate_size (lo, *old_file)) +return -EINVAL ; + +/* +* We might switch to direct I/O mode for the loop device, write back +* all dirty data the page cache now that so that the individual I/O +* operations don't have to do that. +*/ +vfs_fsync (file,0 ); + +/* and ... switch */ +disk_force_media_change (lo->lo_disk); + memflags =blk_mq_freeze_queue (lo->lo_queue); +mapping_set_gfp_mask ((*old_file)->f_mapping, lo->old_gfp_mask); +loop_assign_backing_file (lo, file); +loop_update_dio (lo); +blk_mq_unfreeze_queue (lo->lo_queue, memflags); + *partscan = lo->lo_flags &LO_FLAGS_PARTSCAN ; + +return 0 ; +} +/* * loop_change_fd switched the backing store of a loopback device to * a new file. This is useful for operating system installers to free up
@@ -536,7 +579,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, {struct file *file =fget (arg);struct file *old_file; -unsigned int memflags;int error;bool partscan;bool is_loop;
@@ -557,43 +599,10 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, error =loop_global_lock_killable (lo, is_loop);if (error) goto out_putf; - error = -ENXIO ; -if (lo->lo_state != Lo_bound) - goto out_err; - -/* the loop device has to be read-only */ - error = -EINVAL ; -if (!(lo->lo_flags &LO_FLAGS_READ_ONLY )) - goto out_err; - - error =loop_validate_file (file, bdev); -if (error) - goto out_err; - - old_file = lo->lo_backing_file; - - error = -EINVAL ; - -/* size of the new backing store needs to be the same */ -if (lo_calculate_size (lo, file) !=lo_calculate_size (lo, old_file)) - goto out_err; - -/* -* We might switch to direct I/O mode for the loop device, write back -* all dirty data the page cache now that so that the individual I/O -* operations don't have to do that. -*/ -vfs_fsync (file,0 ); - -/* and ... switch */ -disk_force_media_change (lo->lo_disk); - memflags =blk_mq_freeze_queue (lo->lo_queue); -mapping_set_gfp_mask (old_file->f_mapping, lo->old_gfp_mask); -loop_assign_backing_file (lo, file); -loop_update_dio (lo); -blk_mq_unfreeze_queue (lo->lo_queue, memflags); - partscan = lo->lo_flags &LO_FLAGS_PARTSCAN ; + error =__loop_change_fd (lo, bdev, file, &old_file, &partscan);loop_global_unlock (lo, is_loop); +if (error) + goto out_putf;/* * Flush loop_validate_file() before fput(), for l->lo_backing_file
@@ -618,8 +627,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,kobject_uevent (&disk_to_dev (lo->lo_disk)->kobj,KOBJ_CHANGE );return error; -out_err: -loop_global_unlock (lo, is_loop); out_putf:fput (file);dev_set_uevent_suppress (disk_to_dev (lo->lo_disk),0 );
Re: [PATCH 15/27] loop: Split loop_change_fd()
Quoted 1 line · Bart Van Assche
Prepare for adding a second call of
__loop_change_fd().
A bit confusing message. Yes, you latter add another call, but it's from the same caller to sort out the locking mess, and that is directly relevant to the scope of the factored out helper. So please state that. Same for the next patch. All in merging the first free loop patches would probably be easier to follow.
PATCH 16/27 loop: Split loop_configure()
In reply to Bart Van AsschePrepare for adding a second __loop_configure() call.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/loop.c | 109 ++++++++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 52 deletions(-)
drivers/block/loop.c +57 -52
@@ -981,61 +981,28 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim, lim->discard_granularity =0 ; } -static int loop_configure (struct loop_device *lo,blk_mode_t mode, -struct block_device *bdev, -const struct loop_config *config) +static int __loop_configure (struct loop_device *lo,blk_mode_t mode, +struct block_device *bdev, +const struct loop_config *config,struct file *file, +bool *partscan) { -struct file *file =fget (config->fd);struct queue_limits lim; -int error;loff_t size; -bool partscan; -bool is_loop; - -if (!file) -return -EBADF ; - - error =loop_check_backing_file (file); -if (error) { -fput (file); -return error; - } - - is_loop =is_loop_device (file); - -/* This is safe, since we have a reference from open(). */ -__module_get (THIS_MODULE ); - -/* -* If we don't hold exclusive handle for the device, upgrade to it -* here to avoid changing device under exclusive owner. -*/ -if (!(mode &BLK_OPEN_EXCL )) { - error =bd_prepare_to_claim (bdev, loop_configure,NULL ); -if (error) - goto out_putf; - } - - error =loop_global_lock_killable (lo, is_loop); -if (error) - goto out_bdev; +int error; - error = -EBUSY ;if (lo->lo_state != Lo_unbound) - goto out_unlock; +return -EBUSY ; error =loop_validate_file (file, bdev);if (error) - goto out_unlock; +return error; -if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS ) !=0 ) { - error = -EINVAL ; - goto out_unlock; - } +if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS ) !=0 ) +return -EINVAL ; error =loop_set_status_from_info (lo, &config->info);if (error) - goto out_unlock; +return error; lo->lo_flags = config->info.lo_flags; if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||
@@ -1046,10 +1013,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode, lo->workqueue =alloc_workqueue ("loop%d" ,WQ_UNBOUND |WQ_FREEZABLE ,0 , lo->lo_number); -if (!lo->workqueue) { - error = -ENOMEM ; - goto out_unlock; - } +if (!lo->workqueue) +return -ENOMEM ; }/* suppress uevents while reconfiguring the device */
@@ -1066,7 +1031,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,/* No need to freeze the queue as the device isn't bound yet. */ error =queue_limits_commit_update (lo->lo_queue, &lim);if (error) - goto out_unlock; +return error;/* * We might switch to direct I/O mode for the loop device, write back
@@ -1087,14 +1052,56 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,WRITE_ONCE (lo->lo_state, Lo_bound);if (part_shift) lo->lo_flags |=LO_FLAGS_PARTSCAN ; - partscan = lo->lo_flags &LO_FLAGS_PARTSCAN ; -if (partscan) + *partscan = lo->lo_flags &LO_FLAGS_PARTSCAN ; +if (*partscan)clear_bit (GD_SUPPRESS_PART_SCAN , &lo->lo_disk->state);dev_set_uevent_suppress (disk_to_dev (lo->lo_disk),0 );kobject_uevent (&disk_to_dev (lo->lo_disk)->kobj,KOBJ_CHANGE ); +return 0 ; +} + +static int loop_configure (struct loop_device *lo,blk_mode_t mode, +struct block_device *bdev, +const struct loop_config *config) +{ +struct file *file =fget (config->fd); +int error; +bool partscan; +bool is_loop; + +if (!file) +return -EBADF ; + + error =loop_check_backing_file (file); +if (error) { +fput (file); +return error; + } + + is_loop =is_loop_device (file); + +/* This is safe, since we have a reference from open(). */ +__module_get (THIS_MODULE ); + +/* +* If we don't hold exclusive handle for the device, upgrade to it +* here to avoid changing device under exclusive owner. +*/ +if (!(mode &BLK_OPEN_EXCL )) { + error =bd_prepare_to_claim (bdev, loop_configure,NULL ); +if (error) + goto out_putf; + } + + error =loop_global_lock_killable (lo, is_loop); +if (error) + goto out_bdev; + error =__loop_configure (lo, mode, bdev, config, file, &partscan);loop_global_unlock (lo, is_loop); +if (error) + goto out_bdev;if (partscan)loop_reread_partitions (lo);
@@ -1103,8 +1110,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,return 0 ; -out_unlock: -loop_global_unlock (lo, is_loop); out_bdev:if (!(mode &BLK_OPEN_EXCL ))bd_abort_claiming (bdev, loop_configure);
Re: [PATCH 16/27] loop: Split loop_configure()
Quoted 13 lines · Bart Van Assche
Prepare for adding a second
__loop_configure()call.Signed-off-by Bart Van Assche <[email protected]>--- drivers/block/loop.c | 109 ++++++++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 52 deletions(-)drivers/block/loop.c +0
-0@@ -981,61 +981,28 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim, lim->discard_granularity =0 ;
drivers/block/loop.c static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim, · line 988
Quoted 10 lines
drivers/block/loop.c +4
-3} -static int loop_configure (struct loop_device *lo,blk_mode_t mode, -struct block_device *bdev, -const struct loop_config *config) +static int __loop_configure (struct loop_device *lo,blk_mode_t mode, +struct block_device *bdev, +const struct loop_config *config,struct file *file, +bool *partscan) {
I wonder if we can add "__must_hold(&lo->lo_mutex)" to this. Same for the function __loop_change_fd()
Quoted 151 lines
drivers/block/loop.c +53
-49-struct file *file =fget (config->fd);struct queue_limits lim; -int error;loff_t size; -bool partscan; -bool is_loop; - -if (!file) -return -EBADF ; - - error =loop_check_backing_file (file); -if (error) { -fput (file); -return error; - } - - is_loop =is_loop_device (file); - -/* This is safe, since we have a reference from open(). */ -__module_get (THIS_MODULE ); - -/* -* If we don't hold exclusive handle for the device, upgrade to it -* here to avoid changing device under exclusive owner. -*/ -if (!(mode &BLK_OPEN_EXCL )) { - error =bd_prepare_to_claim (bdev, loop_configure,NULL ); -if (error) - goto out_putf; - } - - error =loop_global_lock_killable (lo, is_loop); -if (error) - goto out_bdev; +int error; - error = -EBUSY ;if (lo->lo_state != Lo_unbound) - goto out_unlock; +return -EBUSY ; error =loop_validate_file (file, bdev);if (error) - goto out_unlock; +return error; -if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS ) !=0 ) { - error = -EINVAL ; - goto out_unlock; - } +if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS ) !=0 ) +return -EINVAL ; error =loop_set_status_from_info (lo, &config->info);if (error) - goto out_unlock; +return error; lo->lo_flags = config->info.lo_flags; if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||@@ -1046,10 +1013,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode, lo->workqueue =alloc_workqueue ("loop%d" ,WQ_UNBOUND |WQ_FREEZABLE ,0 , lo->lo_number); -if (!lo->workqueue) { - error = -ENOMEM ; - goto out_unlock; - } +if (!lo->workqueue) +return -ENOMEM ; }/* suppress uevents while reconfiguring the device */ @@ -1066,7 +1031,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,/* No need to freeze the queue as the device isn't bound yet. */ error =queue_limits_commit_update (lo->lo_queue, &lim);if (error) - goto out_unlock; +return error;/* * We might switch to direct I/O mode for the loop device, write back @@ -1087,14 +1052,56 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,WRITE_ONCE (lo->lo_state, Lo_bound);if (part_shift) lo->lo_flags |=LO_FLAGS_PARTSCAN ; - partscan = lo->lo_flags &LO_FLAGS_PARTSCAN ; -if (partscan) + *partscan = lo->lo_flags &LO_FLAGS_PARTSCAN ; +if (*partscan)clear_bit (GD_SUPPRESS_PART_SCAN , &lo->lo_disk->state);dev_set_uevent_suppress (disk_to_dev (lo->lo_disk),0 );kobject_uevent (&disk_to_dev (lo->lo_disk)->kobj,KOBJ_CHANGE ); +return 0 ; +} + +static int loop_configure (struct loop_device *lo,blk_mode_t mode, +struct block_device *bdev, +const struct loop_config *config) +{ +struct file *file =fget (config->fd); +int error; +bool partscan; +bool is_loop; + +if (!file) +return -EBADF ; + + error =loop_check_backing_file (file); +if (error) { +fput (file); +return error; + } + + is_loop =is_loop_device (file); + +/* This is safe, since we have a reference from open(). */ +__module_get (THIS_MODULE ); + +/* +* If we don't hold exclusive handle for the device, upgrade to it +* here to avoid changing device under exclusive owner. +*/ +if (!(mode &BLK_OPEN_EXCL )) { + error =bd_prepare_to_claim (bdev, loop_configure,NULL ); +if (error) + goto out_putf; + } + + error =loop_global_lock_killable (lo, is_loop); +if (error) + goto out_bdev; + error =__loop_configure (lo, mode, bdev, config, file, &partscan);loop_global_unlock (lo, is_loop); +if (error) + goto out_bdev;if (partscan)loop_reread_partitions (lo);@@ -1103,8 +1110,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,return 0 ; -out_unlock: -loop_global_unlock (lo, is_loop); out_bdev:if (!(mode &BLK_OPEN_EXCL ))bd_abort_claiming (bdev, loop_configure);
Re: [PATCH 16/27] loop: Split loop_configure()
Quoted 12 lines · Haris Iqbal
Quoted 8 lines · Bart Van Assche
drivers/block/loop.c +4
-3-static int loop_configure (struct loop_device *lo,blk_mode_t mode, - struct block_device *bdev, - const struct loop_config *config) +static int __loop_configure (struct loop_device *lo,blk_mode_t mode, + struct block_device *bdev, + const struct loop_config *config, struct file *file, + bool *partscan){
I wonder if we can add "
__must_hold(&lo->lo_mutex)" to this. Same for the function__loop_change_fd()
I will look into this.
Thanks,
Bart.
PATCH 17/27 loop: Remove the "bool global" function argument
In reply to Bart Van AsscheKeep the behavior in loop_global_lock_killable() for the global == true case. Expand loop_global_lock_killable(lo, false) calls into a mutex_lock_killable() and a mutex_unlock() call.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/loop.c | 67 ++++++++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 27 deletions(-)
drivers/block/loop.c +40 -27
@@ -98,25 +98,22 @@ static DEFINE_MUTEX(loop_validate_mutex);* loop_global_lock_killable() - take locks for safe loop_validate_file() test * * @lo: struct loop_device -* @global: true if @lo is about to bind another "struct loop_device", false otherwise * * Returns 0 on success, -EINTR otherwise. * -* Since loop_validate_file() traverses on other "struct loop_device" if -* is_loop_device() is true, we need a global lock for serializing concurrent +* Since loop_validate_file() traverses on other "struct loop_device", we need a +* global lock for serializing concurrent * loop_configure()/loop_change_fd()/__loop_clr_fd() calls. */ -static int loop_global_lock_killable (struct loop_device *lo,bool global) +static int loop_global_lock_killable (struct loop_device *lo) {int err; -if (global) { - err =mutex_lock_killable (&loop_validate_mutex); -if (err) -return err; - } + err =mutex_lock_killable (&loop_validate_mutex); +if (err) +return err; err =mutex_lock_killable (&lo->lo_mutex); -if (err && global) +if (err)mutex_unlock (&loop_validate_mutex);return err; }
@@ -125,13 +122,11 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)* loop_global_unlock() - release locks taken by loop_global_lock_killable() * * @lo: struct loop_device -* @global: true if @lo was about to bind another "struct loop_device", false otherwise */ -static void loop_global_unlock (struct loop_device *lo,bool global) +static void loop_global_unlock (struct loop_device *lo) {mutex_unlock (&lo->lo_mutex); -if (global) -mutex_unlock (&loop_validate_mutex); +mutex_unlock (&loop_validate_mutex); }static int max_part;
@@ -596,11 +591,19 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,dev_set_uevent_suppress (disk_to_dev (lo->lo_disk),1 ); is_loop =is_loop_device (file); - error =loop_global_lock_killable (lo, is_loop); -if (error) - goto out_putf; - error =__loop_change_fd (lo, bdev, file, &old_file, &partscan); -loop_global_unlock (lo, is_loop); +if (is_loop) { + error =loop_global_lock_killable (lo); +if (error) + goto out_putf; + error =__loop_change_fd (lo, bdev, file, &old_file, &partscan); +loop_global_unlock (lo); + }else { + error =mutex_lock_killable (&lo->lo_mutex); +if (error) + goto out_putf; + error =__loop_change_fd (lo, bdev, file, &old_file, &partscan); +mutex_unlock (&lo->lo_mutex); + }if (error) goto out_putf;
@@ -1095,11 +1098,21 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode, goto out_putf; } - error =loop_global_lock_killable (lo, is_loop); -if (error) - goto out_bdev; - error =__loop_configure (lo, mode, bdev, config, file, &partscan); -loop_global_unlock (lo, is_loop); +if (is_loop) { + error =loop_global_lock_killable (lo); +if (error) + goto out_bdev; + error =__loop_configure (lo, mode, bdev, config, file, + &partscan); +loop_global_unlock (lo); + }else { + error =mutex_lock_killable (&lo->lo_mutex); +if (error) + goto out_bdev; + error =__loop_configure (lo, mode, bdev, config, file, + &partscan); +mutex_unlock (&lo->lo_mutex); + }if (error) goto out_bdev;if (partscan)
@@ -1210,11 +1223,11 @@ static int loop_clr_fd(struct loop_device *lo)* which loop_configure()/loop_change_fd() found via fget() was this * loop device. */ - err =loop_global_lock_killable (lo, true); + err =loop_global_lock_killable (lo);if (err)return err;if (lo->lo_state != Lo_bound) { -loop_global_unlock (lo, true); +loop_global_unlock (lo);return -ENXIO ; }/*
@@ -1226,7 +1239,7 @@ static int loop_clr_fd(struct loop_device *lo) lo->lo_flags |=LO_FLAGS_AUTOCLEAR ;if (disk_openers (lo->lo_disk) ==1 )WRITE_ONCE (lo->lo_state, Lo_rundown); -loop_global_unlock (lo, true); +loop_global_unlock (lo);return 0 ; }
PATCH 18/27 loop: Add lock context annotations
In reply to Bart Van AsscheAdd lock context annotations that are compatible with Clang.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/loop.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/block/loop.c +4 -0
@@ -106,6 +106,8 @@ static DEFINE_MUTEX(loop_validate_mutex);* loop_configure()/loop_change_fd()/__loop_clr_fd() calls. */ static int loop_global_lock_killable (struct loop_device *lo) + __cond_acquires(0, &loop_validate_mutex) + __cond_acquires(0, &lo->lo_mutex) {int err;
@@ -124,6 +126,8 @@ static int loop_global_lock_killable(struct loop_device *lo)* @lo: struct loop_device */ static void loop_global_unlock (struct loop_device *lo) + __releases(&lo->lo_mutex) + __releases(&loop_validate_mutex) {mutex_unlock (&lo->lo_mutex);mutex_unlock (&loop_validate_mutex);
Re: [PATCH 18/27] loop: Add lock context annotations
Quoted 22 lines · Bart Van Assche
Add lock context annotations that are compatible with Clang.
Signed-off-by Bart Van Assche <[email protected]>--- drivers/block/loop.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/block/loop.c +2
-0@@ -106,6 +106,8 @@ static DEFINE_MUTEX(loop_validate_mutex);* loop_configure()/loop_change_fd()/__loop_clr_fd() calls. */ static int loop_global_lock_killable (struct loop_device *lo) + __cond_acquires(0, &loop_validate_mutex) + __cond_acquires(0, &lo->lo_mutex) {int err;
drivers/block/loop.c static int loop_global_lock_killable(struct loop_device *lo) · line 133
Quoted 8 lines
drivers/block/loop.c +2
-0@@ -124,6 +126,8 @@ static int loop_global_lock_killable(struct loop_device *lo)* @lo: struct loop_device */ static void loop_global_unlock (struct loop_device *lo) + __releases(&lo->lo_mutex) + __releases(&loop_validate_mutex) {mutex_unlock (&lo->lo_mutex);mutex_unlock (&loop_validate_mutex);
This looks good to me.
One thing I noticed while looking through the loop driver is that it also defines @loop_ctl_mutex, which protects @loop_index_idr. It might be worth annotating @loop_index_idr with `__guarded_by(&loop_ctl_mutex) as well so that Clang can validate accesses to the IDR against the corresponding locking requirements.
Thanks,
--Nilay
Re: [PATCH 18/27] loop: Add lock context annotations
Quoted 6 lines · Nilay Shroff
One thing I noticed while looking through the loop driver is that it also defines @loop_ctl_mutex, which protects @loop_index_idr. It might be worth annotating
@loop_index_idr with `__guarded_by(&loop_ctl_mutex) as well so that Clang can
validate accesses to the IDR against the corresponding locking requirements.
I'm considering to add the changes below as an additional patch:
drivers/block/loop.c +3 -1
@@ -90,8 +90,8 @@ struct loop_cmd {#define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)#define LOOP_DEFAULT_HW_Q_DEPTH 128 -static DEFINE_IDR (loop_index_idr);static DEFINE_MUTEX (loop_ctl_mutex); +static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);static DEFINE_MUTEX (loop_validate_mutex);/**
@@ -2326,6 +2326,8 @@ static void __exit loop_exit(void)struct loop_device *lo;int id; +guard (mutex_init)(&loop_ctl_mutex); +unregister_blkdev (LOOP_MAJOR ,"loop" );misc_deregister (&loop_misc);
Re: [PATCH 18/27] loop: Add lock context annotations
Quoted 25 lines · Bart Van Assche
Quoted 4 lines · Nilay Shroff
One thing I noticed while looking through the loop driver is that it also defines @loop_ctl_mutex, which protects @loop_index_idr. It might be worth annotating @loop_index_idr with `__guarded_by(&loop_ctl_mutex) as well so that Clang can validate accesses to the IDR against the corresponding locking requirements.
I'm considering to add the changes below as an additional patch:
drivers/block/loop.c +1
-1@@ -90,8 +90,8 @@ struct loop_cmd { #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ) #define LOOP_DEFAULT_HW_Q_DEPTH 128 -static DEFINE_IDR (loop_index_idr); static DEFINE_MUTEX(loop_ctl_mutex); +static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr); static DEFINE_MUTEX(loop_validate_mutex); /**
drivers/block/loop.c static void __exit loop_exit(void) · line 2333
Quoted 8 lines
drivers/block/loop.c +2
-0@@ -2326,6 +2326,8 @@ static void __exit loop_exit(void) struct loop_device *lo; int id; + guard(mutex_init)(&loop_ctl_mutex); + unregister_blkdev(LOOP_MAJOR, "loop"); misc_deregister(&loop_misc);
Okay looks good. Alternatively, I think you may also consider updating existing patch with above change while you're adding lock context annotation for loop driver. But anything is fine for me either updating current patch or add it in a new patch.
Thanks,
--Nilay
PATCH 19/27 mtip32: Enable lock context analysis
In reply to Bart Van AsscheLet the compiler verify lock and unlock calls. Enable lock context analysis in the Makefile.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/mtip32xx/Makefile | 2 ++ 1 file changed, 2 insertions(+)
drivers/block/mtip32xx/Makefile +2 -0
@@ -3,4 +3,6 @@ # Makefile for Block device driver for Micron PCIe SSD # +CONTEXT_ANALYSIS := y + obj-$(CONFIG_BLK_DEV_PCIESSD_MTIP32XX) += mtip32xx.o
PATCH 20/27 nbd: Enable lock context analysis
In reply to Bart Van AsscheAdd the missing __must_hold() annotations. Enable lock context analysis in the Makefile.
Reviewed-by Christoph Hellwig <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/nbd.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/block/nbd.c +3 -0
@@ -1469,6 +1469,7 @@ static void nbd_config_put(struct nbd_device *nbd) }static int nbd_start_device (struct nbd_device *nbd) + __must_hold(&nbd->config_lock) {struct nbd_config *config = nbd->config;int num_connections = config->num_connections;
@@ -1541,6 +1542,7 @@ static int nbd_start_device(struct nbd_device *nbd) }static int nbd_start_device_ioctl (struct nbd_device *nbd) + __must_hold(nbd->config_lock) {struct nbd_config *config = nbd->config;int ret;
@@ -1592,6 +1594,7 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)/* Must be called with config_lock held */ static int __nbd_ioctl (struct block_device *bdev,struct nbd_device *nbd,unsigned int cmd,unsigned long arg) + __must_hold(nbd->config_lock) {struct nbd_config *config = nbd->config;loff_t bytesize;
Re: [PATCH 20/27] nbd: Enable lock context analysis
Quoted 31 lines · Bart Van Assche
Add the missing
__must_hold()annotations. Enable lock context analysis in theMakefile.Reviewed-by Christoph Hellwig <[email protected]>Signed-off-by Bart Van Assche <[email protected]>--- drivers/block/nbd.c | 3 +++ 1 file changed, 3 insertions(+)drivers/block/nbd.c +2
-0@@ -1469,6 +1469,7 @@ static void nbd_config_put(struct nbd_device *nbd) }static int nbd_start_device (struct nbd_device *nbd) + __must_hold(&nbd->config_lock) {struct nbd_config *config = nbd->config;int num_connections = config->num_connections;@@ -1541,6 +1542,7 @@ static int nbd_start_device(struct nbd_device *nbd) }static int nbd_start_device_ioctl (struct nbd_device *nbd) + __must_hold(nbd->config_lock) {struct nbd_config *config = nbd->config;int ret;
drivers/block/nbd.c static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout) · line 1600
Quoted 7 lines
drivers/block/nbd.c +1
-0@@ -1592,6 +1594,7 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)/* Must be called with config_lock held */ static int __nbd_ioctl (struct block_device *bdev,struct nbd_device *nbd,unsigned int cmd,unsigned long arg) + __must_hold(nbd->config_lock) {struct nbd_config *config = nbd->config;loff_t bytesize;
Above changes are good, however I see nbd also uses @nbd_index_mutex which guards @nbd_index_idr. So should we also annotate @nbd_index_idr using __guarded_by(&nbd_index_mutex)?
Thanks,
--Nilay
Re: [PATCH 20/27] nbd: Enable lock context analysis
Quoted 3 lines · Nilay Shroff
Above changes are good, however I see nbd also uses @nbd_index_mutex which guards @nbd_index_idr. So should we also annotate @nbd_index_idr using
__guarded_by(&nbd_index_mutex)?
How about adding these changes as an additional patch?
Thanks,
Bart.
drivers/block/nbd.c +4 -2
@@ -49,8 +49,8 @@#define CREATE_TRACE_POINTS #include <trace/events/nbd.h> -static DEFINE_IDR (nbd_index_idr);static DEFINE_MUTEX (nbd_index_mutex); +static __guarded_by(&nbd_index_mutex) DEFINE_IDR(nbd_index_idr);static struct workqueue_struct *nbd_del_wq;static int nbd_total_devices =0 ;
@@ -2739,7 +2739,9 @@ static void __exit nbd_cleanup(void)/* Also wait for nbd_dev_remove_work() completes */ destroy_workqueue (nbd_del_wq); -idr_destroy (&nbd_index_idr); + scoped_guard(mutex_init, &nbd_index_mutex) +idr_destroy (&nbd_index_idr); +unregister_blkdev (NBD_MAJOR ,"nbd" ); }
Re: [PATCH 20/27] nbd: Enable lock context analysis
Quoted 27 lines · Bart Van Assche
Quoted 3 lines · Nilay Shroff
Above changes are good, however I see nbd also uses @nbd_index_mutex which guards @nbd_index_idr. So should we also annotate @nbd_index_idr using
__guarded_by(&nbd_index_mutex)?How about adding these changes as an additional patch?
Thanks,
Bart.
drivers/block/nbd.c +1
-1@@ -49,8 +49,8 @@ #define CREATE_TRACE_POINTS #include <trace/events/nbd.h> -static DEFINE_IDR (nbd_index_idr); static DEFINE_MUTEX(nbd_index_mutex); +static __guarded_by(&nbd_index_mutex) DEFINE_IDR(nbd_index_idr); static struct workqueue_struct *nbd_del_wq; static int nbd_total_devices = 0;
drivers/block/nbd.c static void __exit nbd_cleanup(void) · line 2747
Quoted 10 lines
drivers/block/nbd.c +3
-1@@ -2739,7 +2739,9 @@ static void __exit nbd_cleanup(void) /* Also wait for nbd_dev_remove_work() completes */ destroy_workqueue(nbd_del_wq); - idr_destroy(&nbd_index_idr); + scoped_guard(mutex_init, &nbd_index_mutex) + idr_destroy(&nbd_index_idr); + unregister_blkdev(NBD_MAJOR, "nbd"); }
Looks good. But as I said earlier for similar changes in loop driver, you may want to consider updating current patch (instead of adding an additional patch) with the above changes while you're enabling lock context for nbd driver.
Thanks,
--Nilay
PATCH 21/27 null_blk: Enable lock context analysis
In reply to Bart Van AsscheAdd __must_hold() annotations where these are missing. Annotate two functions that use conditional locking with __context_unsafe(). Enable lock context analysis in the Makefile.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/null_blk/Makefile | 2 ++ drivers/block/null_blk/main.c | 12 ++++++++---- drivers/block/null_blk/zoned.c | 2 ++ 3 files changed, 12 insertions(+), 4 deletions(-)
drivers/block/null_blk/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 +CONTEXT_ANALYSIS := y + # needed for trace events ccflags-y += -I$(src)
drivers/block/null_blk/main.c +8 -4
@@ -1038,6 +1038,7 @@ static struct nullb_page *null_insert_page(struct nullb *nullb, }static int null_flush_cache_page (struct nullb *nullb,struct nullb_page *c_page) + __must_hold(&nullb->lock) {int i;unsigned int offset;
@@ -1087,6 +1088,7 @@ static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page) }static int null_make_cache_space (struct nullb *nullb,unsigned long n) + __must_hold(&nullb->lock) {int i, err, nr_pages;struct nullb_page *c_pages[FREE_BATCH ];
@@ -1139,8 +1141,9 @@ static int null_make_cache_space(struct nullb *nullb, unsigned long n)return 0 ; } -static blk_status_t copy_to_nullb (struct nullb *nullb,void *source, -loff_t pos,size_t n,bool is_fua) +static blk_status_t copy_to_nullb (struct nullb *nullb,void *source,loff_t pos, +size_t n,bool is_fua) + __must_hold(&nullb->lock) {size_t temp, count =0 ;struct nullb_page *t_page;
@@ -1240,8 +1243,9 @@ static blk_status_t null_handle_flush(struct nullb *nullb) }static blk_status_t null_transfer (struct nullb *nullb,struct page *page, -unsigned int len,unsigned int off,bool is_write,loff_t pos, -bool is_fua) +unsigned int len,unsigned int off, +bool is_write,loff_t pos,bool is_fua) + __must_hold(&nullb->lock) {struct nullb_device *dev = nullb->dev;blk_status_t err =BLK_STS_OK ;
drivers/block/null_blk/zoned.c +2 -0
@@ -32,6 +32,7 @@ static inline void null_init_zone_lock(struct nullb_device *dev,static inline void null_lock_zone (struct nullb_device *dev,struct nullb_zone *zone) + __context_unsafe(conditional locking) {if (!dev->memory_backed)spin_lock_irq (&zone->spinlock);
@@ -41,6 +42,7 @@ static inline void null_lock_zone(struct nullb_device *dev,static inline void null_unlock_zone (struct nullb_device *dev,struct nullb_zone *zone) + __context_unsafe(conditional locking) {if (!dev->memory_backed)spin_unlock_irq (&zone->spinlock);
Re: [PATCH 21/27] null_blk: Enable lock context analysis
Quoted 5 lines · Bart Van Assche
Add
__must_hold()annotations where these are missing. Annotate two functions that use conditional locking with__context_unsafe(). Enable lock context analysis in theMakefile.Signed-off-by Bart Van Assche <[email protected]>
Looks OK to me.
Reviewed-by Damien Le Moal <[email protected]>
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH 21/27] null_blk: Enable lock context analysis
In reply to Bart Van AsscheQuoted 2 lines · Bart Van Assche
Add
__must_hold()annotations where these are missing. Annotate two functions that use conditional locking with__context_unsafe().
Please explain why that is needed and there is no better way to have proper annotations. Both here and in a comment in the code.
Re: [PATCH 21/27] null_blk: Enable lock context analysis
Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Bart Van Assche
Add
__must_hold()annotations where these are missing. Annotate two functions that use conditional locking with__context_unsafe().Please explain why that is needed and there is no better way to have proper annotations. Both here and in a comment in the code.
Hi Christoph,
Is the change shown below considered acceptable? It converts the null_lock_zone() and null_unlock_zone() calls into scoped_guard() and thereby eliminates the risk that any null_lock_zone() call would not be paired properly with a null_unlock_zone() call. The DEFINE_CLASS() macro below does the following:
* Before any code block that is protected by scoped_guard(null_zone) or guard(null_zone), call null_lock_zone() and create a local variable with type struct nullb_dev_and_zone. That local variable is declared with the __cleanup__ attribute.
* The __cleanup__ attribute causes null_unlock_zone() to be called with the same arguments as null_lock_zone().
Thanks,
Bart.
drivers/block/null_blk/zoned.c +28 -16
@@ -50,6 +50,23 @@ static inline void null_unlock_zone(struct nullb_device *dev,mutex_unlock (&zone->mutex); } +struct nullb_dev_and_zone { +struct nullb_device *dev; +struct nullb_zone *zone; +}; + +DEFINE_CLASS(null_zone, struct nullb_dev_and_zone, +null_unlock_zone (_T.dev, _T.zone), + ({ +null_lock_zone (dev, zone); + (struct nullb_dev_and_zone ){dev, zone}; + }), + struct nullb_device *dev, struct nullb_zone *zone) + +DEFINE_CLASS_IS_UNCONDITIONAL (null_zone) + int null_init_zoned_dev(struct nullb_device *dev,struct queue_limits *lim) {
@@ -218,14 +235,14 @@ int null_report_zones(struct gendisk *disk, sector_t sector,* So use a local copy to avoid corruption of the device zone * array. */ -null_lock_zone (dev, zone); + scoped_guard(null_zone, dev, zone) { blkz.start = zone->start; blkz.len = zone->len; blkz.wp = zone->wp; blkz.type = zone->type; blkz.cond = zone->cond; blkz.capacity = zone->capacity; -null_unlock_zone (dev, zone); + } error =disk_report_zone (disk, &blkz, i, args);if (error)
@@ -366,7 +383,7 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector,return null_process_cmd (cmd,REQ_OP_WRITE , sector, nr_sectors); } -null_lock_zone (dev, zone); + scoped_guard(null_zone, dev, zone) {/* * Regular writes must be at the write pointer position. Zone append
@@ -446,7 +463,8 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, ret = badblocks_ret; unlock_zone: -null_unlock_zone (dev, zone); + ; + }return ret; }
@@ -657,14 +675,14 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,if (op ==REQ_OP_ZONE_RESET_ALL ) {for (i = dev->zone_nr_conv; i < dev->nr_zones; i++) { zone = &dev->zones[i]; -null_lock_zone (dev, zone); + scoped_guard(null_zone, dev, zone) {if (zone->cond !=BLK_ZONE_COND_EMPTY && zone->cond !=BLK_ZONE_COND_READONLY && zone->cond !=BLK_ZONE_COND_OFFLINE ) {null_reset_zone (dev, zone);trace_nullb_zone_op (cmd, i, zone->cond); } -null_unlock_zone (dev, zone); + } }return BLK_STS_OK ; }
@@ -672,7 +690,7 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op, zone_no =null_zone_no (dev, sector); zone = &dev->zones[zone_no]; -null_lock_zone (dev, zone); + scoped_guard(null_zone, dev, zone) {if (zone->cond ==BLK_ZONE_COND_READONLY || zone->cond ==BLK_ZONE_COND_OFFLINE ) {
@@ -702,7 +720,8 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_op op,trace_nullb_zone_op (cmd, zone_no, zone->cond); unlock: -null_unlock_zone (dev, zone); + ; + }return ret; }
@@ -712,7 +731,6 @@ blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_op op, {struct nullb_device *dev;struct nullb_zone *zone; -blk_status_t sts;switch (op) {case REQ_OP_WRITE :
@@ -731,10 +749,8 @@ blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_op op,if (zone->cond ==BLK_ZONE_COND_OFFLINE )return BLK_STS_IOERR ; -null_lock_zone (dev, zone); - sts =null_process_cmd (cmd, op, sector, nr_sectors); -null_unlock_zone (dev, zone); -return sts; + scoped_guard(null_zone, dev, zone) +return null_process_cmd (cmd, op, sector, nr_sectors); } }
@@ -748,7 +764,7 @@ static void null_set_zone_cond(struct nullb_device *dev, cond != BLK_ZONE_COND_OFFLINE))return ; -null_lock_zone (dev, zone); +guard (null_zone)(dev, zone);/* * If the read-only condition is requested again to zones already in
@@ -769,8 +785,6 @@ static void null_set_zone_cond(struct nullb_device *dev, zone->cond = cond; zone->wp =NULL_ZONE_INVALID_WP ; } - -null_unlock_zone (dev, zone); }/*
Re: [PATCH 21/27] null_blk: Enable lock context analysis
In reply to Bart Van AsscheQuoted 6 lines · Bart Van Assche
Add
__must_hold()annotations where these are missing. Annotate two functions that use conditional locking with__context_unsafe(). Enable lock context analysis in theMakefile.Signed-off-by Bart Van Assche<[email protected]>---
Looks good to me.
Reviewed-by Nilay Shroff <[email protected]>
PATCH 22/27 rbd: Enable lock context analysis
In reply to Bart Van AsscheAdd lock context annotations and enable lock context analysis.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/rbd.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/block/rbd.c +8 -0
@@ -4185,6 +4185,7 @@ static void rbd_acquire_lock(struct work_struct *work) }static bool rbd_quiesce_lock (struct rbd_device *rbd_dev) + __must_hold(&rbd_dev->lock_rwsem) {dout ("%s rbd_dev %p\n" , __func__, rbd_dev);lockdep_assert_held_write (&rbd_dev->lock_rwsem);
@@ -4229,6 +4230,7 @@ static void __rbd_release_lock(struct rbd_device *rbd_dev)* lock_rwsem must be held for write */ static void rbd_release_lock (struct rbd_device *rbd_dev) + __must_hold(&rbd_dev->lock_rwsem) {if (!rbd_quiesce_lock (rbd_dev))return ;
@@ -4585,6 +4587,7 @@ static void rbd_unregister_watch(struct rbd_device *rbd_dev)* lock_rwsem must be held for write */ static void rbd_reacquire_lock (struct rbd_device *rbd_dev) + __must_hold(&rbd_dev->lock_rwsem) {struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;char cookie[32 ];
@@ -6782,6 +6785,7 @@ static void rbd_dev_device_release(struct rbd_device *rbd_dev)* upon return. */ static int rbd_dev_device_setup (struct rbd_device *rbd_dev) + __releases(&rbd_dev->header_rwsem) {int ret;
@@ -6883,6 +6887,7 @@ static void rbd_dev_image_release(struct rbd_device *rbd_dev)* with @depth == 0. */ static int rbd_dev_image_probe (struct rbd_device *rbd_dev,int depth) + __context_unsafe(conditional locking on @depth) {bool need_watch = !rbd_is_ro (rbd_dev);int ret;
@@ -7136,6 +7141,9 @@ static ssize_t do_rbd_add(const char *buf, size_t count)if (rc <0 ) goto err_out_rbd_dev; +/* Acquired by rbd_dev_image_probe(rbd_dev, 0) */ +__acquire (&rbd_dev->header_rwsem); +if (rbd_dev->opts->alloc_size > rbd_dev->layout.object_size) {rbd_warn (rbd_dev,"alloc_size adjusted to %u" , rbd_dev->layout.object_size);
Re: [PATCH 22/27] rbd: Enable lock context analysis
Quoted 1 line · Bart Van Assche
Add lock context annotations and enable lock context analysis.
It does part one, it does not do part 2 described here.
PATCH 23/27 ublk: Enable lock context analysis
In reply to Bart Van AsscheAdd the lock context annotations that are required by Clang.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/ublk_drv.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/block/ublk_drv.c +6 -0
@@ -374,11 +374,13 @@ static inline bool ublk_support_batch_io(const struct ublk_queue *ubq) }static inline void ublk_io_lock (struct ublk_io *io) + __acquires(&io->lock) {spin_lock (&io->lock); }static inline void ublk_io_unlock (struct ublk_io *io) + __releases(&io->lock) {spin_unlock (&io->lock); }
@@ -3263,6 +3265,7 @@ static int ublk_check_fetch_buf(const struct ublk_device *ub, __u64 buf_addr)static int __ublk_fetch (struct io_uring_cmd *cmd,struct ublk_device *ub,struct ublk_io *io,u16 q_id) + __must_hold(&ub->mutex) {/* UBLK_IO_FETCH_REQ is only allowed before dev is setup */ if (ublk_dev_ready (ub))
@@ -3686,6 +3689,7 @@ static void ublk_batch_revert_prep_cmd(struct ublk_batch_io_iter *iter,static int ublk_batch_prep_io (struct ublk_queue *ubq,const struct ublk_batch_io_data *data,const struct ublk_elem_header *elem) + __must_hold(&data->ub->mutex) {struct ublk_io *io = &ubq->ios[elem->tag];const struct ublk_batch_io *uc = &data->header;
@@ -5290,6 +5294,7 @@ static int ublk_char_dev_permission(struct ublk_device *ub,* already holds ub->mutex when calling del_gendisk() which freezes the queue. */ static unsigned int ublk_lock_buf_tree (struct ublk_device *ub) + __acquires(&ub->mutex) {unsigned int memflags =0 ;
@@ -5301,6 +5306,7 @@ static unsigned int ublk_lock_buf_tree(struct ublk_device *ub) }static void ublk_unlock_buf_tree (struct ublk_device *ub,unsigned int memflags) + __releases(&ub->mutex) {if (ub->ub_disk)blk_mq_unfreeze_queue (ub->ub_disk->queue, memflags);
Re: [PATCH 23/27] ublk: Enable lock context analysis
Quoted 3 lines · Bart Van Assche
Add the lock context annotations that are required by Clang.
Signed-off-by Bart Van Assche <[email protected]>
Reviewed-by Ming Lei <[email protected]>
Thanks,
Ming
PATCH 24/27 xen-blkback: Enable lock context analysis
In reply to Bart Van AsscheLet Clang verify lock and unlock calls. Enable lock context analysis in the Makefile.
Acked-by Roger Pau Monné <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/xen-blkback/Makefile | 3 +++ 1 file changed, 3 insertions(+)
drivers/block/xen-blkback/Makefile +3 -0
@@ -1,4 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only + +CONTEXT_ANALYSIS := y + obj-$(CONFIG_XEN_BLKDEV_BACKEND) := xen-blkback.o xen-blkback-y := blkback.o xenbus.o
PATCH 25/27 zram: Enable lock context analysis
In reply to Bart Van AsscheAdd the lock context annotations that are required by Clang. Enable lock context analysis in the Makefile.
Reviewed-by Sergey Senozhatsky <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/zram/Makefile | 2 ++ drivers/block/zram/zcomp.c | 3 ++- drivers/block/zram/zcomp.h | 6 ++++-- 3 files changed, 8 insertions(+), 3 deletions(-)
drivers/block/zram/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only +CONTEXT_ANALYSIS := y + zram-y := zcomp.o zram_drv.o zram-$(CONFIG_ZRAM_BACKEND_LZO) += backend_lzorle.o backend_lzo.o
drivers/block/zram/zcomp.c +2 -1
@@ -112,7 +112,8 @@ ssize_t zcomp_available_show(const char *comp, char *buf, ssize_t at)return at; } -struct zcomp_strm *zcomp_stream_get (struct zcomp *comp) +struct zcomp_strm *__zcomp_stream_get (struct zcomp *comp) + __context_unsafe(uses raw_cpu_ptr()) {for (;;) {struct zcomp_strm *zstrm =raw_cpu_ptr (comp->stream);
drivers/block/zram/zcomp.h +4 -2
@@ -85,8 +85,10 @@ const char *zcomp_lookup_backend_name(const char *comp);struct zcomp *zcomp_create (const char *alg,struct zcomp_params *params);void zcomp_destroy (struct zcomp *comp); -struct zcomp_strm *zcomp_stream_get (struct zcomp *comp); -void zcomp_stream_put (struct zcomp_strm *zstrm); +#define zcomp_stream_get (...) __acquire_ret(__zcomp_stream_get(__VA_ARGS__), &__ret->lock) +struct zcomp_strm *__zcomp_stream_get (struct zcomp *comp); +void zcomp_stream_put(struct zcomp_strm *zstrm) + __releases(&zstrm->lock);int zcomp_compress (struct zcomp *comp,struct zcomp_strm *zstrm,const void *src,unsigned int *dst_len);
PATCH 26/27 rnbd: Enable lock context analysis
In reply to Bart Van AsscheLet Clang verify lock and unlock calls. Enable lock context analysis in the Makefile.
Acked-by Jack Wang <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/rnbd/Makefile | 2 ++ 1 file changed, 2 insertions(+)
drivers/block/rnbd/Makefile +2 -0
@@ -1,5 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-or-later +CONTEXT_ANALYSIS := y + ccflags-y := -I$(srctree)/drivers/infiniband/ulp/rtrs rnbd-client-y := rnbd-clt.o \
PATCH 27/27 block: Enable lock context analysis for all block drivers
In reply to Bart Van AsscheNow that all locking functions in block drivers have been annotated, enable lock context analysis for all block drivers at the top level of drivers/block/.
Signed-off-by Bart Van Assche <[email protected]>
--- drivers/block/Makefile | 2 ++ 1 file changed, 2 insertions(+)
drivers/block/Makefile +2 -0
@@ -6,6 +6,8 @@ # Rewritten to use lists instead of if-statements. # +CONTEXT_ANALYSIS := y + # needed for trace events ccflags-y += -I$(src)
Can you please split your series up in a saner way? A bunch of the patches for drivers with their own subdirectories could be merged ASAP, but you bundle them with lots of unrelated bits. Similarly having a standalone series for drbd probably makes life easier as you don't have to CC random other driver authors on it.
Marco: is there a way to enable the context analysis for parts of a directory? I'm not sure that always converting a whole directory is going to be feasible in the long run.
Quoted 10 lines · Christoph Hellwig
Can you please split your series up in a saner way? A bunch of the patches for drivers with their own subdirectories could be merged ASAP, but you bundle them with lots of unrelated bits. Similarly having a standalone series for drbd probably makes life easier as you don't have to CC random other driver authors on it.
Marco: is there a way to enable the context analysis for parts of a directory? I'm not sure that always converting a whole directory is going to be feasible in the long run.
Yes, just:
CONTEXT_ANALYSIS_foo.o := y
CONTEXT_ANALYSIS_bar.o := y
...
Alternatively, if most are to be enabled, and only a few holdouts:
CONTEXT_ANALYSIS := y
CONTEXT_ANALYSIS_foo.o := n
CONTEXT_ANALYSIS_bar.o := n
Cool, thanks!
Bart: maytbe for next version just enable it on a per-driver basis. Once all are covered we can switch to directory-wide.
Quoted 2 lines · Christoph Hellwig
Bart: maytbe for next version just enable it on a per-driver basis. Once all are covered we can switch to directory-wide.
I can do that. Since the merge window likely will open this Monday, I probably should wait with reposting any patches from this series until after the merge window has closed.
Thanks for having taken the time to review this patch series.
Bart.