[PATCH 0/10] udf: Unify aops

[PATCH 0/10] udf: Unify aops

[Jan Kara]

Hello,

this patch series makes UDF use the same address_space_operations for both
normal and in-ICB files as switching aops on live files is prone to races
as spotted by syzbot. When already dealing with this code, switch readpage
function from using kmap_atomic() to use kmap_local_page().

I plan to queue these patches to my tree.

								Honza

[PATCH 01/10] udf: Unify .read_folio for normal and in-ICB files

[Jan Kara]

Switching address_space_operations while a file is used is difficult to
do in a race-free way. To be able to use single address_space_operations
in UDF, make udf_read_folio() handle both normal and in-ICB files.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 15 +++------------
 fs/udf/inode.c   |  9 ++++++++-
 fs/udf/udfdecl.h |  2 ++
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 322115c8369d..2666234a5204 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -38,7 +38,7 @@
 #include "udf_i.h"
 #include "udf_sb.h"
 
-static void __udf_adinicb_readpage(struct page *page)
+void udf_adinicb_readpage(struct page *page)
 {
 	struct inode *inode = page->mapping->host;
 	char *kaddr;
@@ -57,15 +57,6 @@ static void __udf_adinicb_readpage(struct page *page)
 	kunmap_atomic(kaddr);
 }
 
-static int udf_adinicb_read_folio(struct file *file, struct folio *folio)
-{
-	BUG_ON(!folio_test_locked(folio));
-	__udf_adinicb_readpage(&folio->page);
-	folio_unlock(folio);
-
-	return 0;
-}
-
 static int udf_adinicb_writepage(struct page *page,
 				 struct writeback_control *wbc)
 {
@@ -100,7 +91,7 @@ static int udf_adinicb_write_begin(struct file *file,
 	*pagep = page;
 
 	if (!PageUptodate(page))
-		__udf_adinicb_readpage(page);
+		udf_adinicb_readpage(page);
 	return 0;
 }
 
@@ -127,7 +118,7 @@ static int udf_adinicb_write_end(struct file *file, struct address_space *mappin
 const struct address_space_operations udf_adinicb_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
-	.read_folio	= udf_adinicb_read_folio,
+	.read_folio	= udf_read_folio,
 	.writepage	= udf_adinicb_writepage,
 	.write_begin	= udf_adinicb_write_begin,
 	.write_end	= udf_adinicb_write_end,
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 15e0d9f23c06..9ef56574e452 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -191,8 +191,15 @@ static int udf_writepages(struct address_space *mapping,
 	return mpage_writepages(mapping, wbc, udf_get_block_wb);
 }
 
-static int udf_read_folio(struct file *file, struct folio *folio)
+int udf_read_folio(struct file *file, struct folio *folio)
 {
+	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
+
+	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB) {
+		udf_adinicb_readpage(&folio->page);
+		folio_unlock(folio);
+		return 0;
+	}
 	return mpage_read_folio(folio, udf_get_block);
 }
 
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 5ba59ab90d48..6b93b393cb46 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -138,6 +138,7 @@ static inline unsigned int udf_dir_entry_len(struct fileIdentDesc *cfi)
 
 /* file.c */
 extern long udf_ioctl(struct file *, unsigned int, unsigned long);
+void udf_adinicb_readpage(struct page *page);
 
 /* inode.c */
 extern struct inode *__udf_iget(struct super_block *, struct kernel_lb_addr *,
@@ -158,6 +159,7 @@ extern struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
 extern int udf_setsize(struct inode *, loff_t);
 extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
+int udf_read_folio(struct file *file, struct folio *folio);
 extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
 			 struct kernel_lb_addr *, uint32_t *, sector_t *);
 int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
-- 
2.35.3

Re: [PATCH 01/10] udf: Unify .read_folio for normal and in-ICB files

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 02/10] udf: Convert in-ICB files to use udf_writepages()

[Jan Kara]

Switching address_space_operations while a file is used is difficult to
do in a race-free way. To be able to use single address_space_operations
in UDF, make in-ICB files use udf_writepages().

Reported-by: syzbot+c27475eb921c46bbdc62@syzkaller.appspotmail.com
Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 21 +--------------------
 fs/udf/inode.c   | 29 ++++++++++++++++++++++++++---
 fs/udf/udfdecl.h |  1 +
 3 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 2666234a5204..7a8dbad86e41 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -57,25 +57,6 @@ void udf_adinicb_readpage(struct page *page)
 	kunmap_atomic(kaddr);
 }
 
-static int udf_adinicb_writepage(struct page *page,
-				 struct writeback_control *wbc)
-{
-	struct inode *inode = page->mapping->host;
-	char *kaddr;
-	struct udf_inode_info *iinfo = UDF_I(inode);
-
-	BUG_ON(!PageLocked(page));
-
-	kaddr = kmap_atomic(page);
-	memcpy(iinfo->i_data + iinfo->i_lenEAttr, kaddr, i_size_read(inode));
-	SetPageUptodate(page);
-	kunmap_atomic(kaddr);
-	mark_inode_dirty(inode);
-	unlock_page(page);
-
-	return 0;
-}
-
 static int udf_adinicb_write_begin(struct file *file,
 			struct address_space *mapping, loff_t pos,
 			unsigned len, struct page **pagep,
@@ -119,7 +100,7 @@ const struct address_space_operations udf_adinicb_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= udf_read_folio,
-	.writepage	= udf_adinicb_writepage,
+	.writepages	= udf_writepages,
 	.write_begin	= udf_adinicb_write_begin,
 	.write_end	= udf_adinicb_write_end,
 	.direct_IO	= udf_adinicb_direct_IO,
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 9ef56574e452..54e6127ebf55 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -185,10 +185,33 @@ static void udf_write_failed(struct address_space *mapping, loff_t to)
 	}
 }
 
-static int udf_writepages(struct address_space *mapping,
-			struct writeback_control *wbc)
+static int udf_adinicb_writepage(struct page *page,
+				 struct writeback_control *wbc, void *data)
 {
-	return mpage_writepages(mapping, wbc, udf_get_block_wb);
+	struct inode *inode = page->mapping->host;
+	char *kaddr;
+	struct udf_inode_info *iinfo = UDF_I(inode);
+
+	BUG_ON(!PageLocked(page));
+
+	kaddr = kmap_atomic(page);
+	memcpy(iinfo->i_data + iinfo->i_lenEAttr, kaddr, i_size_read(inode));
+	SetPageUptodate(page);
+	kunmap_atomic(kaddr);
+	unlock_page(page);
+	mark_inode_dirty(inode);
+
+	return 0;
+}
+
+int udf_writepages(struct address_space *mapping, struct writeback_control *wbc)
+{
+	struct inode *inode = mapping->host;
+	struct udf_inode_info *iinfo = UDF_I(inode);
+
+	if (iinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB)
+		return mpage_writepages(mapping, wbc, udf_get_block_wb);
+	return write_cache_pages(mapping, wbc, udf_adinicb_writepage, NULL);
 }
 
 int udf_read_folio(struct file *file, struct folio *folio)
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 6b93b393cb46..48647eab26a6 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -160,6 +160,7 @@ extern int udf_setsize(struct inode *, loff_t);
 extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
 int udf_read_folio(struct file *file, struct folio *folio);
+int udf_writepages(struct address_space *mapping, struct writeback_control *wbc);
 extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
 			 struct kernel_lb_addr *, uint32_t *, sector_t *);
 int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
-- 
2.35.3

Re: [PATCH 02/10] udf: Convert in-ICB files to use udf_writepages()

[Christoph Hellwig]

As a pure mechanical move this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But some comments:

> +	struct inode *inode = page->mapping->host;
> +	char *kaddr;
> +	struct udf_inode_info *iinfo = UDF_I(inode);
> +
> +	BUG_ON(!PageLocked(page));
> +
> +	kaddr = kmap_atomic(page);
> +	memcpy(iinfo->i_data + iinfo->i_lenEAttr, kaddr, i_size_read(inode));
> +	SetPageUptodate(page);
> +	kunmap_atomic(kaddr);
> +	unlock_page(page);

This really should be using memcpy_to_page.  And the SetPageUptodate
here in ->writepages loos a little odd as well.

Re: [PATCH 02/10] udf: Convert in-ICB files to use udf_writepages()

[Jan Kara]

On Tue 24-01-23 05:25:09, Christoph Hellwig wrote:
> As a pure mechanical move this looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

> But some comments:
> 
> > +	struct inode *inode = page->mapping->host;
> > +	char *kaddr;
> > +	struct udf_inode_info *iinfo = UDF_I(inode);
> > +
> > +	BUG_ON(!PageLocked(page));
> > +
> > +	kaddr = kmap_atomic(page);
> > +	memcpy(iinfo->i_data + iinfo->i_lenEAttr, kaddr, i_size_read(inode));
> > +	SetPageUptodate(page);
> > +	kunmap_atomic(kaddr);
> > +	unlock_page(page);
> 
> This really should be using memcpy_to_page.  And the SetPageUptodate
> here in ->writepages loos a little odd as well.

Good points. Added a cleanup patch for this. And then one more to get rid
of the last remaining kmap_atomic() use in UDF (BTW that would benefit from
memcpy_to_page_page() helper as well).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 03/10] udf: Convert in-ICB files to use udf_direct_IO()

[Jan Kara]

Switching address_space_operations while a file is used is difficult to
do in a race-free way. To be able to use single address_space_operations
in UDF, make in-ICB files use udf_direct_IO().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 8 +-------
 fs/udf/inode.c   | 5 ++++-
 fs/udf/udfdecl.h | 1 +
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 7a8dbad86e41..6bab6aa7770a 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -76,12 +76,6 @@ static int udf_adinicb_write_begin(struct file *file,
 	return 0;
 }
 
-static ssize_t udf_adinicb_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
-{
-	/* Fallback to buffered I/O. */
-	return 0;
-}
-
 static int udf_adinicb_write_end(struct file *file, struct address_space *mapping,
 				 loff_t pos, unsigned len, unsigned copied,
 				 struct page *page, void *fsdata)
@@ -103,7 +97,7 @@ const struct address_space_operations udf_adinicb_aops = {
 	.writepages	= udf_writepages,
 	.write_begin	= udf_adinicb_write_begin,
 	.write_end	= udf_adinicb_write_end,
-	.direct_IO	= udf_adinicb_direct_IO,
+	.direct_IO	= udf_direct_IO,
 };
 
 static vm_fault_t udf_page_mkwrite(struct vm_fault *vmf)
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 54e6127ebf55..52016c942f68 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -243,7 +243,7 @@ static int udf_write_begin(struct file *file, struct address_space *mapping,
 	return ret;
 }
 
-static ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
+ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
 	struct address_space *mapping = file->f_mapping;
@@ -251,6 +251,9 @@ static ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	size_t count = iov_iter_count(iter);
 	ssize_t ret;
 
+	/* Fallback to buffered IO for in-ICB files */
+	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
+		return 0;
 	ret = blockdev_direct_IO(iocb, inode, iter, udf_get_block);
 	if (unlikely(ret < 0 && iov_iter_rw(iter) == WRITE))
 		udf_write_failed(mapping, iocb->ki_pos + count);
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 48647eab26a6..a851613465c6 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -161,6 +161,7 @@ extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
 int udf_read_folio(struct file *file, struct folio *folio);
 int udf_writepages(struct address_space *mapping, struct writeback_control *wbc);
+ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
 			 struct kernel_lb_addr *, uint32_t *, sector_t *);
 int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
-- 
2.35.3

Re: [PATCH 03/10] udf: Convert in-ICB files to use udf_direct_IO()

[Christoph Hellwig]

On Tue, Jan 24, 2023 at 01:06:14PM +0100, Jan Kara wrote:
> Switching address_space_operations while a file is used is difficult to
> do in a race-free way. To be able to use single address_space_operations
> in UDF, make in-ICB files use udf_direct_IO().

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 04/10] udf: Convert in-ICB files to use udf_write_begin()

[Jan Kara]

Switching address_space_operations while a file is used is difficult to
do in a race-free way. To be able to use single address_space_operations
in UDF, make in-ICB files use udf_write_begin().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 21 +--------------------
 fs/udf/inode.c   | 24 +++++++++++++++++++-----
 fs/udf/udfdecl.h |  3 +++
 3 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 6bab6aa7770a..16aecf4b2387 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -57,25 +57,6 @@ void udf_adinicb_readpage(struct page *page)
 	kunmap_atomic(kaddr);
 }
 
-static int udf_adinicb_write_begin(struct file *file,
-			struct address_space *mapping, loff_t pos,
-			unsigned len, struct page **pagep,
-			void **fsdata)
-{
-	struct page *page;
-
-	if (WARN_ON_ONCE(pos >= PAGE_SIZE))
-		return -EIO;
-	page = grab_cache_page_write_begin(mapping, 0);
-	if (!page)
-		return -ENOMEM;
-	*pagep = page;
-
-	if (!PageUptodate(page))
-		udf_adinicb_readpage(page);
-	return 0;
-}
-
 static int udf_adinicb_write_end(struct file *file, struct address_space *mapping,
 				 loff_t pos, unsigned len, unsigned copied,
 				 struct page *page, void *fsdata)
@@ -95,7 +76,7 @@ const struct address_space_operations udf_adinicb_aops = {
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= udf_read_folio,
 	.writepages	= udf_writepages,
-	.write_begin	= udf_adinicb_write_begin,
+	.write_begin	= udf_write_begin,
 	.write_end	= udf_adinicb_write_end,
 	.direct_IO	= udf_direct_IO,
 };
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 52016c942f68..7f5b2b1f9847 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -231,16 +231,30 @@ static void udf_readahead(struct readahead_control *rac)
 	mpage_readahead(rac, udf_get_block);
 }
 
-static int udf_write_begin(struct file *file, struct address_space *mapping,
+int udf_write_begin(struct file *file, struct address_space *mapping,
 			loff_t pos, unsigned len,
 			struct page **pagep, void **fsdata)
 {
+	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
+	struct page *page;
 	int ret;
 
-	ret = block_write_begin(mapping, pos, len, pagep, udf_get_block);
-	if (unlikely(ret))
-		udf_write_failed(mapping, pos + len);
-	return ret;
+	if (iinfo->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
+		ret = block_write_begin(mapping, pos, len, pagep,
+					udf_get_block);
+		if (unlikely(ret))
+			udf_write_failed(mapping, pos + len);
+		return ret;
+	}
+	if (WARN_ON_ONCE(pos >= PAGE_SIZE))
+		return -EIO;
+	page = grab_cache_page_write_begin(mapping, 0);
+	if (!page)
+		return -ENOMEM;
+	*pagep = page;
+	if (!PageUptodate(page))
+		udf_adinicb_readpage(page);
+	return 0;
 }
 
 ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index a851613465c6..32decf6b6a21 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -161,6 +161,9 @@ extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
 int udf_read_folio(struct file *file, struct folio *folio);
 int udf_writepages(struct address_space *mapping, struct writeback_control *wbc);
+int udf_write_begin(struct file *file, struct address_space *mapping,
+			loff_t pos, unsigned len,
+			struct page **pagep, void **fsdata);
 ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
 			 struct kernel_lb_addr *, uint32_t *, sector_t *);
-- 
2.35.3

Re: [PATCH 04/10] udf: Convert in-ICB files to use udf_write_begin()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 05/10] udf: Convert all file types to use udf_write_end()

[Jan Kara]

Switching address_space_operations while a file is used is difficult to
do in a race-free way. To be able to use single address_space_operations
in UDF, create udf_write_end() function that is able to handle both
normal and in-ICB files.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 16 +---------------
 fs/udf/inode.c   | 22 +++++++++++++++++++++-
 fs/udf/udfdecl.h |  3 +++
 3 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 16aecf4b2387..8a37cd593883 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -57,27 +57,13 @@ void udf_adinicb_readpage(struct page *page)
 	kunmap_atomic(kaddr);
 }
 
-static int udf_adinicb_write_end(struct file *file, struct address_space *mapping,
-				 loff_t pos, unsigned len, unsigned copied,
-				 struct page *page, void *fsdata)
-{
-	struct inode *inode = page->mapping->host;
-	loff_t last_pos = pos + copied;
-	if (last_pos > inode->i_size)
-		i_size_write(inode, last_pos);
-	set_page_dirty(page);
-	unlock_page(page);
-	put_page(page);
-	return copied;
-}
-
 const struct address_space_operations udf_adinicb_aops = {
 	.dirty_folio	= block_dirty_folio,
 	.invalidate_folio = block_invalidate_folio,
 	.read_folio	= udf_read_folio,
 	.writepages	= udf_writepages,
 	.write_begin	= udf_write_begin,
-	.write_end	= udf_adinicb_write_end,
+	.write_end	= udf_write_end,
 	.direct_IO	= udf_direct_IO,
 };
 
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 7f5b2b1f9847..91758c8d77e5 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -257,6 +257,26 @@ int udf_write_begin(struct file *file, struct address_space *mapping,
 	return 0;
 }
 
+int udf_write_end(struct file *file, struct address_space *mapping,
+		  loff_t pos, unsigned len, unsigned copied,
+		  struct page *page, void *fsdata)
+{
+	struct inode *inode = file_inode(file);
+	loff_t last_pos;
+
+	if (UDF_I(inode)->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB)
+		return generic_write_end(file, mapping, pos, len, copied, page,
+					 fsdata);
+	last_pos = pos + copied;
+	if (last_pos > inode->i_size)
+		i_size_write(inode, last_pos);
+	set_page_dirty(page);
+	unlock_page(page);
+	put_page(page);
+
+	return copied;
+}
+
 ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
@@ -286,7 +306,7 @@ const struct address_space_operations udf_aops = {
 	.readahead	= udf_readahead,
 	.writepages	= udf_writepages,
 	.write_begin	= udf_write_begin,
-	.write_end	= generic_write_end,
+	.write_end	= udf_write_end,
 	.direct_IO	= udf_direct_IO,
 	.bmap		= udf_bmap,
 	.migrate_folio	= buffer_migrate_folio,
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 32decf6b6a21..304c2ec81589 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -164,6 +164,9 @@ int udf_writepages(struct address_space *mapping, struct writeback_control *wbc)
 int udf_write_begin(struct file *file, struct address_space *mapping,
 			loff_t pos, unsigned len,
 			struct page **pagep, void **fsdata);
+int udf_write_end(struct file *file, struct address_space *mapping,
+		  loff_t pos, unsigned len, unsigned copied,
+		  struct page *page, void *fsdata);
 ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
 			 struct kernel_lb_addr *, uint32_t *, sector_t *);
-- 
2.35.3

Re: [PATCH 05/10] udf: Convert all file types to use udf_write_end()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 06/10] udf: Add handling of in-ICB files to udf_bmap()

[Jan Kara]

Add detection of in-ICB files to udf_bmap() and return error in that
case. This will allow us o use single address_space_operations in UDF.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 91758c8d77e5..703db2a4516b 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -296,6 +296,10 @@ ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 static sector_t udf_bmap(struct address_space *mapping, sector_t block)
 {
+	struct udf_inode_info *iinfo = UDF_I(mapping->host);
+
+	if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
+		return -EINVAL;
 	return generic_block_bmap(mapping, block, udf_get_block);
 }
 
-- 
2.35.3

Re: [PATCH 06/10] udf: Add handling of in-ICB files to udf_bmap()

[Christoph Hellwig]

On Tue, Jan 24, 2023 at 01:06:17PM +0100, Jan Kara wrote:
> Add detection of in-ICB files to udf_bmap() and return error in that
> case. This will allow us o use single address_space_operations in UDF.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 07/10] udf: Switch to single address_space_operations

[Jan Kara]

Now that udf_aops and udf_adiniicb_aops are functionally identical, just
drop udf_adiniicb_aops.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 10 ----------
 fs/udf/inode.c   |  8 +-------
 fs/udf/namei.c   | 10 ++--------
 fs/udf/udfdecl.h |  1 -
 4 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 8a37cd593883..84e0b241940d 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -57,16 +57,6 @@ void udf_adinicb_readpage(struct page *page)
 	kunmap_atomic(kaddr);
 }
 
-const struct address_space_operations udf_adinicb_aops = {
-	.dirty_folio	= block_dirty_folio,
-	.invalidate_folio = block_invalidate_folio,
-	.read_folio	= udf_read_folio,
-	.writepages	= udf_writepages,
-	.write_begin	= udf_write_begin,
-	.write_end	= udf_write_end,
-	.direct_IO	= udf_direct_IO,
-};
-
 static vm_fault_t udf_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 703db2a4516b..5b0be8f281f0 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -364,8 +364,6 @@ int udf_expand_file_adinicb(struct inode *inode)
 		iinfo->i_alloc_type = ICBTAG_FLAG_AD_SHORT;
 	else
 		iinfo->i_alloc_type = ICBTAG_FLAG_AD_LONG;
-	/* from now on we have normal address_space methods */
-	inode->i_data.a_ops = &udf_aops;
 	set_page_dirty(page);
 	unlock_page(page);
 	up_write(&iinfo->i_data_sem);
@@ -379,7 +377,6 @@ int udf_expand_file_adinicb(struct inode *inode)
 		kunmap_atomic(kaddr);
 		unlock_page(page);
 		iinfo->i_alloc_type = ICBTAG_FLAG_AD_IN_ICB;
-		inode->i_data.a_ops = &udf_adinicb_aops;
 		iinfo->i_lenAlloc = inode->i_size;
 		up_write(&iinfo->i_data_sem);
 	}
@@ -1566,10 +1563,7 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
 	case ICBTAG_FILE_TYPE_REGULAR:
 	case ICBTAG_FILE_TYPE_UNDEF:
 	case ICBTAG_FILE_TYPE_VAT20:
-		if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
-			inode->i_data.a_ops = &udf_adinicb_aops;
-		else
-			inode->i_data.a_ops = &udf_aops;
+		inode->i_data.a_ops = &udf_aops;
 		inode->i_op = &udf_file_inode_operations;
 		inode->i_fop = &udf_file_operations;
 		inode->i_mode |= S_IFREG;
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 1b0f4c600b63..c043584463d2 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -373,10 +373,7 @@ static int udf_create(struct user_namespace *mnt_userns, struct inode *dir,
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 
-	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
-		inode->i_data.a_ops = &udf_adinicb_aops;
-	else
-		inode->i_data.a_ops = &udf_aops;
+	inode->i_data.a_ops = &udf_aops;
 	inode->i_op = &udf_file_inode_operations;
 	inode->i_fop = &udf_file_operations;
 	mark_inode_dirty(inode);
@@ -392,10 +389,7 @@ static int udf_tmpfile(struct user_namespace *mnt_userns, struct inode *dir,
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 
-	if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
-		inode->i_data.a_ops = &udf_adinicb_aops;
-	else
-		inode->i_data.a_ops = &udf_aops;
+	inode->i_data.a_ops = &udf_aops;
 	inode->i_op = &udf_file_inode_operations;
 	inode->i_fop = &udf_file_operations;
 	mark_inode_dirty(inode);
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 304c2ec81589..d8c0de3b224e 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -80,7 +80,6 @@ extern const struct inode_operations udf_file_inode_operations;
 extern const struct file_operations udf_file_operations;
 extern const struct inode_operations udf_symlink_inode_operations;
 extern const struct address_space_operations udf_aops;
-extern const struct address_space_operations udf_adinicb_aops;
 extern const struct address_space_operations udf_symlink_aops;
 
 struct udf_fileident_iter {
-- 
2.35.3

Re: [PATCH 07/10] udf: Switch to single address_space_operations

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 08/10] udf: Mark aops implementation static

[Jan Kara]

Mark functions implementing aops static since they are not needed
outside of inode.c anymore.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c   | 19 ++++++++++---------
 fs/udf/udfdecl.h |  9 ---------
 2 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 5b0be8f281f0..af069a05919c 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -204,7 +204,8 @@ static int udf_adinicb_writepage(struct page *page,
 	return 0;
 }
 
-int udf_writepages(struct address_space *mapping, struct writeback_control *wbc)
+static int udf_writepages(struct address_space *mapping,
+			  struct writeback_control *wbc)
 {
 	struct inode *inode = mapping->host;
 	struct udf_inode_info *iinfo = UDF_I(inode);
@@ -214,7 +215,7 @@ int udf_writepages(struct address_space *mapping, struct writeback_control *wbc)
 	return write_cache_pages(mapping, wbc, udf_adinicb_writepage, NULL);
 }
 
-int udf_read_folio(struct file *file, struct folio *folio)
+static int udf_read_folio(struct file *file, struct folio *folio)
 {
 	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
 
@@ -231,9 +232,9 @@ static void udf_readahead(struct readahead_control *rac)
 	mpage_readahead(rac, udf_get_block);
 }
 
-int udf_write_begin(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len,
-			struct page **pagep, void **fsdata)
+static int udf_write_begin(struct file *file, struct address_space *mapping,
+			   loff_t pos, unsigned len,
+			   struct page **pagep, void **fsdata)
 {
 	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
 	struct page *page;
@@ -257,9 +258,9 @@ int udf_write_begin(struct file *file, struct address_space *mapping,
 	return 0;
 }
 
-int udf_write_end(struct file *file, struct address_space *mapping,
-		  loff_t pos, unsigned len, unsigned copied,
-		  struct page *page, void *fsdata)
+static int udf_write_end(struct file *file, struct address_space *mapping,
+			 loff_t pos, unsigned len, unsigned copied,
+			 struct page *page, void *fsdata)
 {
 	struct inode *inode = file_inode(file);
 	loff_t last_pos;
@@ -277,7 +278,7 @@ int udf_write_end(struct file *file, struct address_space *mapping,
 	return copied;
 }
 
-ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
+static ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
 	struct address_space *mapping = file->f_mapping;
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index d8c0de3b224e..337daf97d5b4 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -158,15 +158,6 @@ extern struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
 extern int udf_setsize(struct inode *, loff_t);
 extern void udf_evict_inode(struct inode *);
 extern int udf_write_inode(struct inode *, struct writeback_control *wbc);
-int udf_read_folio(struct file *file, struct folio *folio);
-int udf_writepages(struct address_space *mapping, struct writeback_control *wbc);
-int udf_write_begin(struct file *file, struct address_space *mapping,
-			loff_t pos, unsigned len,
-			struct page **pagep, void **fsdata);
-int udf_write_end(struct file *file, struct address_space *mapping,
-		  loff_t pos, unsigned len, unsigned copied,
-		  struct page *page, void *fsdata);
-ssize_t udf_direct_IO(struct kiocb *iocb, struct iov_iter *iter);
 extern int8_t inode_bmap(struct inode *, sector_t, struct extent_position *,
 			 struct kernel_lb_addr *, uint32_t *, sector_t *);
 int udf_get_block(struct inode *, sector_t, struct buffer_head *, int);
-- 
2.35.3

Re: [PATCH 08/10] udf: Mark aops implementation static

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 09/10] udf: Move udf_adinicb_readpage() to inode.c

[Jan Kara]

udf_adinicb_readpage() is only called from aops functions, move it to
the same file as its callers and also drop the stale comment -
invalidate_lock is protecting us against races with truncate.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/file.c    | 19 -------------------
 fs/udf/inode.c   | 15 +++++++++++++++
 fs/udf/udfdecl.h |  1 -
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/fs/udf/file.c b/fs/udf/file.c
index 84e0b241940d..26592577b7da 100644
--- a/fs/udf/file.c
+++ b/fs/udf/file.c
@@ -38,25 +38,6 @@
 #include "udf_i.h"
 #include "udf_sb.h"
 
-void udf_adinicb_readpage(struct page *page)
-{
-	struct inode *inode = page->mapping->host;
-	char *kaddr;
-	struct udf_inode_info *iinfo = UDF_I(inode);
-	loff_t isize = i_size_read(inode);
-
-	/*
-	 * We have to be careful here as truncate can change i_size under us.
-	 * So just sample it once and use the same value everywhere.
-	 */
-	kaddr = kmap_atomic(page);
-	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
-	memset(kaddr + isize, 0, PAGE_SIZE - isize);
-	flush_dcache_page(page);
-	SetPageUptodate(page);
-	kunmap_atomic(kaddr);
-}
-
 static vm_fault_t udf_page_mkwrite(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index af069a05919c..dcd3f1dac227 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -215,6 +215,21 @@ static int udf_writepages(struct address_space *mapping,
 	return write_cache_pages(mapping, wbc, udf_adinicb_writepage, NULL);
 }
 
+static void udf_adinicb_readpage(struct page *page)
+{
+	struct inode *inode = page->mapping->host;
+	char *kaddr;
+	struct udf_inode_info *iinfo = UDF_I(inode);
+	loff_t isize = i_size_read(inode);
+
+	kaddr = kmap_atomic(page);
+	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
+	memset(kaddr + isize, 0, PAGE_SIZE - isize);
+	flush_dcache_page(page);
+	SetPageUptodate(page);
+	kunmap_atomic(kaddr);
+}
+
 static int udf_read_folio(struct file *file, struct folio *folio)
 {
 	struct udf_inode_info *iinfo = UDF_I(file_inode(file));
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 337daf97d5b4..88692512a466 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -137,7 +137,6 @@ static inline unsigned int udf_dir_entry_len(struct fileIdentDesc *cfi)
 
 /* file.c */
 extern long udf_ioctl(struct file *, unsigned int, unsigned long);
-void udf_adinicb_readpage(struct page *page);
 
 /* inode.c */
 extern struct inode *__udf_iget(struct super_block *, struct kernel_lb_addr *,
-- 
2.35.3

Re: [PATCH 09/10] udf: Move udf_adinicb_readpage() to inode.c

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 10/10] udf: Switch udf_adinicb_readpage() to kmap_local_page()

[Jan Kara]

Instead of using kmap_atomic() use kmap_local_page() in
udf_adinicb_readpage().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index dcd3f1dac227..5ae29f89869b 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -222,12 +222,12 @@ static void udf_adinicb_readpage(struct page *page)
 	struct udf_inode_info *iinfo = UDF_I(inode);
 	loff_t isize = i_size_read(inode);
 
-	kaddr = kmap_atomic(page);
+	kaddr = kmap_local_page(page);
 	memcpy(kaddr, iinfo->i_data + iinfo->i_lenEAttr, isize);
 	memset(kaddr + isize, 0, PAGE_SIZE - isize);
 	flush_dcache_page(page);
 	SetPageUptodate(page);
-	kunmap_atomic(kaddr);
+	kunmap_local(kaddr);
 }
 
 static int udf_read_folio(struct file *file, struct folio *folio)
-- 
2.35.3

Re: [PATCH 10/10] udf: Switch udf_adinicb_readpage() to kmap_local_page()

[Christoph Hellwig]

On Tue, Jan 24, 2023 at 01:06:21PM +0100, Jan Kara wrote:
> Instead of using kmap_atomic() use kmap_local_page() in
> udf_adinicb_readpage().

Looks good.  Given how often this pattern is repeated I wonder if
we want a memcpy_to_page_and_pad helper, though.

Reviewed-by: Christoph Hellwig <hch@lst.de>