Re: [PATCH RFC 4/5] ext2fs: parallel bitmap loading

["Theodore Y. Ts'o" <tytso@mit.edu>]

On Thu, Dec 10, 2020 at 05:12:09PM -0700, Andreas Dilger wrote:
> > @@ -329,12 +369,20 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
> > 				}
> > 				if (!bitmap_tail_verify((unsigned char *) block_bitmap,
> > 							block_nbytes, fs->blocksize - 1))
> > -					tail_flags |= EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> > +					*tail_flags |= EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> > 			} else
> > 				memset(block_bitmap, 0, block_nbytes);
> > 			cnt = block_nbytes << 3;
> > +#ifdef HAVE_PTHREAD
> > +			if (mutex)
> > +				pthread_mutex_lock(mutex);
> > +#endif
> > 			retval = ext2fs_set_block_bitmap_range2(fs->block_map,
> > 					       blk_itr, cnt, block_bitmap);
> > +#ifdef HAVE_PTHREAD
> > +			if (mutex)
> > +				pthread_mutex_unlock(mutex);
> > +#endif
> 
> (style) It wouldn't be terrible to have wrappers around these functions
> instead of inline #ifdef in the few places they are used, like:
> 
> #ifdef HAVE_PTHREAD
> static void unix_pthread_mutex_lock(pthread_mutex_t *mutex)
> {
> 	if (mutex)
> 		pthread_mutex_lock(mutex);
> }
> static void unix_pthread_mutex_unlock(pthread_mutex_t *mutex)
> {
> 	if (mutex)
> 		pthread_mutex_unlock(mutex);
> }
> #else
> #define unix_pthread_mutex_lock(mutex) do {} while (0)
> #define unix_pthread_mutex_unlock(mutex) do {} while (0)
> #endif

We'd also need to have a typedef for mutex_t which is either
pthreads_mutex_t if pthreads are available, or an int (or some other
placeholder type) if it isn't.

I had tried to make sure that rw_bitmaps.c will correctly compile with
HAVE_PTHREAD and HAVE_PTHREAD_H are undefined.  It looks like I didn't
quite get it completely working since there's at leasts one function
signature where we have an unprotected use of pthread_mutex_t, so
that's something we should check before finalizing the patch --- in
addition to the unprotected use of pthread_mutex_{lock,unlock} that
you pointed out.

      		    	   	 		   - Ted