diff mbox series

[3/7] minix: don't flush page immediately for DIRSYNC directories

Message ID 20230108165645.381077-4-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/7] btrfs: don't read the disk superblock for zoned devices in btrfs_scratch_superblocks | expand

Commit Message

Christoph Hellwig Jan. 8, 2023, 4:56 p.m. UTC 
We do not need to writeout modified directory blocks immediately when
modifying them while the page is locked. It is enough to do the flush
somewhat later which has the added benefit that inode times can be
flushed as well. It also allows us to stop depending on
write_one_page() function.

Ported from an ext2 patch by Jan Kara.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/minix/dir.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

Comments

Matthew Wilcox Jan. 8, 2023, 9:17 p.m. UTC | #1 
On Sun, Jan 08, 2023 at 05:56:41PM +0100, Christoph Hellwig wrote:
> @@ -274,9 +280,10 @@ int minix_add_link(struct dentry *dentry, struct inode *inode)
>  		memset (namx + namelen, 0, sbi->s_dirsize - namelen - 2);
>  		de->inode = inode->i_ino;
>  	}
> -	err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> +	dir_commit_chunk(page, pos, sbi->s_dirsize);
>  	dir->i_mtime = dir->i_ctime = current_time(dir);
>  	mark_inode_dirty(dir);
> +	minix_handle_dirsync(dir);

Doesn't this need to be:

	err = minix_handle_dirsync(dir);

> @@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
>  			((minix3_dirent *) de)->inode = inode->i_ino;
>  		else
>  			de->inode = inode->i_ino;
> -		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> +		dir_commit_chunk(page, pos, sbi->s_dirsize);
>  	} else {
>  		unlock_page(page);
>  	}
> -- 

Aren't you missing a call to minix_handle_dirsync() in this function?
Christoph Hellwig Jan. 10, 2023, 8:22 a.m. UTC | #2 
On Sun, Jan 08, 2023 at 09:17:26PM +0000, Matthew Wilcox wrote:
> > +	dir_commit_chunk(page, pos, sbi->s_dirsize);
> >  	dir->i_mtime = dir->i_ctime = current_time(dir);
> >  	mark_inode_dirty(dir);
> > +	minix_handle_dirsync(dir);
> 
> Doesn't this need to be:
> 
> 	err = minix_handle_dirsync(dir);

Yes, fixed.

> 
> > @@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
> >  			((minix3_dirent *) de)->inode = inode->i_ino;
> >  		else
> >  			de->inode = inode->i_ino;
> > -		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> > +		dir_commit_chunk(page, pos, sbi->s_dirsize);
> >  	} else {
> >  		unlock_page(page);
> >  	}
> > -- 
> 
> Aren't you missing a call to minix_handle_dirsync() in this function?

Yes, fixed.
Al Viro Jan. 11, 2023, 2:20 a.m. UTC | #3 
On Tue, Jan 10, 2023 at 09:22:25AM +0100, Christoph Hellwig wrote:
> On Sun, Jan 08, 2023 at 09:17:26PM +0000, Matthew Wilcox wrote:
> > > +	dir_commit_chunk(page, pos, sbi->s_dirsize);
> > >  	dir->i_mtime = dir->i_ctime = current_time(dir);
> > >  	mark_inode_dirty(dir);
> > > +	minix_handle_dirsync(dir);
> > 
> > Doesn't this need to be:
> > 
> > 	err = minix_handle_dirsync(dir);
> 
> Yes, fixed.
> 
> > 
> > > @@ -426,7 +436,7 @@ void minix_set_link(struct minix_dir_entry *de, struct page *page,
> > >  			((minix3_dirent *) de)->inode = inode->i_ino;
> > >  		else
> > >  			de->inode = inode->i_ino;
> > > -		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
> > > +		dir_commit_chunk(page, pos, sbi->s_dirsize);
> > >  	} else {
> > >  		unlock_page(page);
> > >  	}
> > > -- 
> > 
> > Aren't you missing a call to minix_handle_dirsync() in this function?
> 
> Yes, fixed.

More seriously, all those ..._set_link() need to return an error and their
callers (..._rename()) need to deal with failures.  That goes for ext2
as well, and that part is worth splitting off into a prereq - it's a -stable
fodder.
Christoph Hellwig Jan. 11, 2023, 4:26 a.m. UTC | #4 
On Wed, Jan 11, 2023 at 02:20:41AM +0000, Al Viro wrote:
> More seriously, all those ..._set_link() need to return an error and their
> callers (..._rename()) need to deal with failures.

That's actually what I did yesterday:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/remove-write_one_page
Al Viro Jan. 11, 2023, 4:58 a.m. UTC | #5 
On Wed, Jan 11, 2023 at 05:26:41AM +0100, Christoph Hellwig wrote:
> On Wed, Jan 11, 2023 at 02:20:41AM +0000, Al Viro wrote:
> > More seriously, all those ..._set_link() need to return an error and their
> > callers (..._rename()) need to deal with failures.
> 
> That's actually what I did yesterday:
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/remove-write_one_page

ext2 also has that bug.  As well as "need to check for delete_entry errors"
one (also in ext2_rename()).

Completely untested patch follows:

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index e5cbc27ba459..b38fab33cd0d 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -461,7 +461,7 @@ static int ext2_handle_dirsync(struct inode *dir)
 	return err;
 }
 
-void ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
+int ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
 		   struct page *page, void *page_addr, struct inode *inode,
 		   int update_times)
 {
@@ -480,7 +480,7 @@ void ext2_set_link(struct inode *dir, struct ext2_dir_entry_2 *de,
 		dir->i_mtime = dir->i_ctime = current_time(dir);
 	EXT2_I(dir)->i_flags &= ~EXT2_BTREE_FL;
 	mark_inode_dirty(dir);
-	ext2_handle_dirsync(dir);
+	return ext2_handle_dirsync(dir);
 }
 
 /*
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 28de11a22e5f..95c083bb1b7c 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -734,7 +734,7 @@ extern int ext2_delete_entry(struct ext2_dir_entry_2 *dir, struct page *page,
 			     char *kaddr);
 extern int ext2_empty_dir (struct inode *);
 extern struct ext2_dir_entry_2 *ext2_dotdot(struct inode *dir, struct page **p, void **pa);
-extern void ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
+extern int ext2_set_link(struct inode *, struct ext2_dir_entry_2 *, struct page *, void *,
 			  struct inode *, int);
 static inline void ext2_put_page(struct page *page, void *page_addr)
 {
diff --git a/fs/ext2/namei.c b/fs/ext2/namei.c
index c056957221a2..5e3397680faa 100644
--- a/fs/ext2/namei.c
+++ b/fs/ext2/namei.c
@@ -370,8 +370,10 @@ static int ext2_rename (struct user_namespace * mnt_userns,
 			err = PTR_ERR(new_de);
 			goto out_dir;
 		}
-		ext2_set_link(new_dir, new_de, new_page, page_addr, old_inode, 1);
+		err = ext2_set_link(new_dir, new_de, new_page, page_addr, old_inode, 1);
 		ext2_put_page(new_page, page_addr);
+		if (err)
+			goto out_dir;
 		new_inode->i_ctime = current_time(new_inode);
 		if (dir_de)
 			drop_nlink(new_inode);
@@ -391,7 +393,9 @@ static int ext2_rename (struct user_namespace * mnt_userns,
 	old_inode->i_ctime = current_time(old_inode);
 	mark_inode_dirty(old_inode);
 
-	ext2_delete_entry(old_de, old_page, old_page_addr);
+	err = ext2_delete_entry(old_de, old_page, old_page_addr);
+	if (err)
+		goto out_dir;
 
 	if (dir_de) {
 		if (old_dir != new_dir)
diff mbox series

Patch

diff --git a/fs/minix/dir.c b/fs/minix/dir.c
index dcfe5b25378b54..d48b09271dc48f 100644
--- a/fs/minix/dir.c
+++ b/fs/minix/dir.c
@@ -46,21 +46,27 @@  minix_last_byte(struct inode *inode, unsigned long page_nr)
 	return last_byte;
 }
 
-static int dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
+static void dir_commit_chunk(struct page *page, loff_t pos, unsigned len)
 {
 	struct address_space *mapping = page->mapping;
 	struct inode *dir = mapping->host;
-	int err = 0;
+
 	block_write_end(NULL, mapping, pos, len, len, page, NULL);
 
 	if (pos+len > dir->i_size) {
 		i_size_write(dir, pos+len);
 		mark_inode_dirty(dir);
 	}
-	if (IS_DIRSYNC(dir))
-		err = write_one_page(page);
-	else
-		unlock_page(page);
+	unlock_page(page);
+}
+
+static int minix_handle_dirsync(struct inode *dir)
+{
+	int err;
+
+	err = filemap_write_and_wait(dir->i_mapping);
+	if (!err)
+		err = sync_inode_metadata(dir, 1);
 	return err;
 }
 
@@ -274,9 +280,10 @@  int minix_add_link(struct dentry *dentry, struct inode *inode)
 		memset (namx + namelen, 0, sbi->s_dirsize - namelen - 2);
 		de->inode = inode->i_ino;
 	}
-	err = dir_commit_chunk(page, pos, sbi->s_dirsize);
+	dir_commit_chunk(page, pos, sbi->s_dirsize);
 	dir->i_mtime = dir->i_ctime = current_time(dir);
 	mark_inode_dirty(dir);
+	minix_handle_dirsync(dir);
 out_put:
 	dir_put_page(page);
 out:
@@ -302,13 +309,15 @@  int minix_delete_entry(struct minix_dir_entry *de, struct page *page)
 			((minix3_dirent *) de)->inode = 0;
 		else
 			de->inode = 0;
-		err = dir_commit_chunk(page, pos, len);
+		dir_commit_chunk(page, pos, len);
 	} else {
 		unlock_page(page);
 	}
 	dir_put_page(page);
 	inode->i_ctime = inode->i_mtime = current_time(inode);
 	mark_inode_dirty(inode);
+	if (!err)
+		err = minix_handle_dirsync(inode);
 	return err;
 }
 
@@ -349,7 +358,8 @@  int minix_make_empty(struct inode *inode, struct inode *dir)
 	}
 	kunmap_atomic(kaddr);
 
-	err = dir_commit_chunk(page, 0, 2 * sbi->s_dirsize);
+	dir_commit_chunk(page, 0, 2 * sbi->s_dirsize);
+	err = minix_handle_dirsync(inode);
 fail:
 	put_page(page);
 	return err;
@@ -426,7 +436,7 @@  void minix_set_link(struct minix_dir_entry *de, struct page *page,
 			((minix3_dirent *) de)->inode = inode->i_ino;
 		else
 			de->inode = inode->i_ino;
-		err = dir_commit_chunk(page, pos, sbi->s_dirsize);
+		dir_commit_chunk(page, pos, sbi->s_dirsize);
 	} else {
 		unlock_page(page);
 	}