This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
("UBIFS: Add security.* XATTR support for the UBIFS").
As UBIFS does not use generic inode xattr inode operations
the code behind sb->s_xattr is never called.
Remove that dead code for now.
Cc: Subodh Nijsure <[email protected]>
Cc: Marc Kleine-Budde <[email protected]>
Cc: Brad Mouring <[email protected]>
Cc: Gratian Crisan <[email protected]>
Cc: Artem Bityutskiy <[email protected]>
Reported-by: Andreas Grünbacher <[email protected]>
Signed-off-by: Richard Weinberger <[email protected]>
---
After the merge window (and my vacation) I'll have the time to
re-introduce/work security xattr support.
Thanks,
//richard
---
fs/ubifs/super.c | 1 -
fs/ubifs/ubifs.h | 1 -
fs/ubifs/xattr.c | 40 ----------------------------------------
3 files changed, 42 deletions(-)
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 9547a278..c71edca 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -2037,7 +2037,6 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
if (c->max_inode_sz > MAX_LFS_FILESIZE)
sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE;
sb->s_op = &ubifs_super_operations;
- sb->s_xattr = ubifs_xattr_handlers;
mutex_lock(&c->umount_mutex);
err = mount_ubifs(c);
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index de75902..33b6ee7 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1470,7 +1470,6 @@ extern spinlock_t ubifs_infos_lock;
extern atomic_long_t ubifs_clean_zn_cnt;
extern struct kmem_cache *ubifs_inode_slab;
extern const struct super_operations ubifs_super_operations;
-extern const struct xattr_handler *ubifs_xattr_handlers[];
extern const struct address_space_operations ubifs_file_address_operations;
extern const struct file_operations ubifs_file_operations;
extern const struct inode_operations ubifs_file_inode_operations;
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 96f3448..b512b14 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -582,46 +582,6 @@ out_free:
return err;
}
-static size_t security_listxattr(struct dentry *d, char *list, size_t list_size,
- const char *name, size_t name_len, int flags)
-{
- const int prefix_len = XATTR_SECURITY_PREFIX_LEN;
- const size_t total_len = prefix_len + name_len + 1;
-
- if (list && total_len <= list_size) {
- memcpy(list, XATTR_SECURITY_PREFIX, prefix_len);
- memcpy(list + prefix_len, name, name_len);
- list[prefix_len + name_len] = '\0';
- }
-
- return total_len;
-}
-
-static int security_getxattr(struct dentry *d, const char *name, void *buffer,
- size_t size, int flags)
-{
- return ubifs_getxattr(d, name, buffer, size);
-}
-
-static int security_setxattr(struct dentry *d, const char *name,
- const void *value, size_t size, int flags,
- int handler_flags)
-{
- return ubifs_setxattr(d, name, value, size, flags);
-}
-
-static const struct xattr_handler ubifs_xattr_security_handler = {
- .prefix = XATTR_SECURITY_PREFIX,
- .list = security_listxattr,
- .get = security_getxattr,
- .set = security_setxattr,
-};
-
-const struct xattr_handler *ubifs_xattr_handlers[] = {
- &ubifs_xattr_security_handler,
- NULL,
-};
-
static int init_xattrs(struct inode *inode, const struct xattr *xattr_array,
void *fs_info)
{
--
1.8.4.5
Currently UBIFS does not support direct IO, but some applications
blindly use the O_DIRECT flag.
Instead of failing upon open() we can do better and fall back
to buffered IO.
Cc: Dongsheng Yang <[email protected]>
Cc: [email protected]
Suggested-by: Dave Chinner <[email protected]>
Signed-off-by: Richard Weinberger <[email protected]>
---
fs/ubifs/file.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index a3dfe2a..a61fe86 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1540,6 +1540,15 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
return 0;
}
+/*
+ * For now fall back to buffered IO.
+ */
+static ssize_t ubifs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
+ loff_t offset)
+{
+ return 0;
+}
+
const struct address_space_operations ubifs_file_address_operations = {
.readpage = ubifs_readpage,
.writepage = ubifs_writepage,
@@ -1548,6 +1557,7 @@ const struct address_space_operations ubifs_file_address_operations = {
.invalidatepage = ubifs_invalidatepage,
.set_page_dirty = ubifs_set_page_dirty,
.releasepage = ubifs_releasepage,
+ .direct_IO = ubifs_direct_IO,
};
const struct inode_operations ubifs_file_inode_operations = {
--
1.8.4.5
On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
> ("UBIFS: Add security.* XATTR support for the UBIFS").
Hi Richard,
What about a full reverting of this commit. In ubifs, we
*can* support any namespace of xattr including user, trusted, security
or other anyone prefixed by any words. But we have a check_namespace()
in xattr.c to limit what we want to support. That said, if we want to
"Add security.* XATTR support for the UBIFS", what we need to do is
just extending the check_namespace() to allow security namespace pass.
And yes, check_namespace() have been supporting security namespace.
So, IMHO, we do not depend on the generic mechanism at all, and we can
fully revert this commit.
But strange to me, why we picked this commit for ubifs? Artem, is there
something I am missing?
Yang
>
> As UBIFS does not use generic inode xattr inode operations
> the code behind sb->s_xattr is never called.
> Remove that dead code for now.
>
> Cc: Subodh Nijsure <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Cc: Brad Mouring <[email protected]>
> Cc: Gratian Crisan <[email protected]>
> Cc: Artem Bityutskiy <[email protected]>
> Reported-by: Andreas Grünbacher <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> After the merge window (and my vacation) I'll have the time to
> re-introduce/work security xattr support.
>
> Thanks,
> //richard
> ---
> fs/ubifs/super.c | 1 -
> fs/ubifs/ubifs.h | 1 -
> fs/ubifs/xattr.c | 40 ----------------------------------------
> 3 files changed, 42 deletions(-)
>
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 9547a278..c71edca 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -2037,7 +2037,6 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
> if (c->max_inode_sz > MAX_LFS_FILESIZE)
> sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE;
> sb->s_op = &ubifs_super_operations;
> - sb->s_xattr = ubifs_xattr_handlers;
>
> mutex_lock(&c->umount_mutex);
> err = mount_ubifs(c);
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index de75902..33b6ee7 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1470,7 +1470,6 @@ extern spinlock_t ubifs_infos_lock;
> extern atomic_long_t ubifs_clean_zn_cnt;
> extern struct kmem_cache *ubifs_inode_slab;
> extern const struct super_operations ubifs_super_operations;
> -extern const struct xattr_handler *ubifs_xattr_handlers[];
> extern const struct address_space_operations ubifs_file_address_operations;
> extern const struct file_operations ubifs_file_operations;
> extern const struct inode_operations ubifs_file_inode_operations;
> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
> index 96f3448..b512b14 100644
> --- a/fs/ubifs/xattr.c
> +++ b/fs/ubifs/xattr.c
> @@ -582,46 +582,6 @@ out_free:
> return err;
> }
>
> -static size_t security_listxattr(struct dentry *d, char *list, size_t list_size,
> - const char *name, size_t name_len, int flags)
> -{
> - const int prefix_len = XATTR_SECURITY_PREFIX_LEN;
> - const size_t total_len = prefix_len + name_len + 1;
> -
> - if (list && total_len <= list_size) {
> - memcpy(list, XATTR_SECURITY_PREFIX, prefix_len);
> - memcpy(list + prefix_len, name, name_len);
> - list[prefix_len + name_len] = '\0';
> - }
> -
> - return total_len;
> -}
> -
> -static int security_getxattr(struct dentry *d, const char *name, void *buffer,
> - size_t size, int flags)
> -{
> - return ubifs_getxattr(d, name, buffer, size);
> -}
> -
> -static int security_setxattr(struct dentry *d, const char *name,
> - const void *value, size_t size, int flags,
> - int handler_flags)
> -{
> - return ubifs_setxattr(d, name, value, size, flags);
> -}
> -
> -static const struct xattr_handler ubifs_xattr_security_handler = {
> - .prefix = XATTR_SECURITY_PREFIX,
> - .list = security_listxattr,
> - .get = security_getxattr,
> - .set = security_setxattr,
> -};
> -
> -const struct xattr_handler *ubifs_xattr_handlers[] = {
> - &ubifs_xattr_security_handler,
> - NULL,
> -};
> -
> static int init_xattrs(struct inode *inode, const struct xattr *xattr_array,
> void *fs_info)
> {
>
On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> Currently UBIFS does not support direct IO, but some applications
> blindly use the O_DIRECT flag.
> Instead of failing upon open() we can do better and fall back
> to buffered IO.
Hmmmm, to be honest, I am not sure we have to do it as Dave
suggested. I think that's just a work-around for current fstests.
IMHO, perform a buffered IO when user request direct IO without
any warning sounds not a good idea. Maybe adding a warning would
make it better.
I think we need more discussion about AIO&DIO in ubifs, and actually
I have a plan for it. But I have not listed the all cons and pros of
it so far.
Artem, what's your opinion?
Yang
>
> Cc: Dongsheng Yang <[email protected]>
> Cc: [email protected]
> Suggested-by: Dave Chinner <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> fs/ubifs/file.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index a3dfe2a..a61fe86 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1540,6 +1540,15 @@ static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma)
> return 0;
> }
>
> +/*
> + * For now fall back to buffered IO.
> + */
> +static ssize_t ubifs_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
> + loff_t offset)
> +{
> + return 0;
> +}
> +
> const struct address_space_operations ubifs_file_address_operations = {
> .readpage = ubifs_readpage,
> .writepage = ubifs_writepage,
> @@ -1548,6 +1557,7 @@ const struct address_space_operations ubifs_file_address_operations = {
> .invalidatepage = ubifs_invalidatepage,
> .set_page_dirty = ubifs_set_page_dirty,
> .releasepage = ubifs_releasepage,
> + .direct_IO = ubifs_direct_IO,
> };
>
> const struct inode_operations ubifs_file_inode_operations = {
>
Yang, (Sorry if I've used your last name lately)
Am 20.08.2015 um 05:00 schrieb Dongsheng Yang:
> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
>> Currently UBIFS does not support direct IO, but some applications
>> blindly use the O_DIRECT flag.
>> Instead of failing upon open() we can do better and fall back
>> to buffered IO.
>
> Hmmmm, to be honest, I am not sure we have to do it as Dave
> suggested. I think that's just a work-around for current fstests.
>
> IMHO, perform a buffered IO when user request direct IO without
> any warning sounds not a good idea. Maybe adding a warning would
> make it better.
Well, how would you inform the user?
A printk() to dmesg is useless are the vast majority of open()
callers do not check dmesg... :)
Major filesystems implement ->direct_IO these days and having
a "return 0"-stub seems to be legit.
For example exofs does too. So, I really don't consider it a work around.
> I think we need more discussion about AIO&DIO in ubifs, and actually
> I have a plan for it. But I have not listed the all cons and pros of
> it so far.
Sure, having a real ->direct_IO would be be best solution.
My patch won't block this.
Thanks,
//richard
On Thu, 2015-08-20 at 10:48 +0800, Dongsheng Yang wrote:
> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> > This is a partial revert of commit
> > d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
> > ("UBIFS: Add security.* XATTR support for the UBIFS").
>
> Hi Richard,
> What about a full reverting of this commit. In ubifs, we
> *can* support any namespace of xattr including user, trusted,
> security
> or other anyone prefixed by any words. But we have a
> check_namespace()
> in xattr.c to limit what we want to support. That said, if we want to
> "Add security.* XATTR support for the UBIFS", what we need to do is
> just extending the check_namespace() to allow security namespace
> pass.
> And yes, check_namespace() have been supporting security namespace.
>
> So, IMHO, we do not depend on the generic mechanism at all, and we
> can
> fully revert this commit.
We just weren't careful enough.
--
Best Regards,
Artem Bityutskiy
Am 20.08.2015 um 04:48 schrieb Dongsheng Yang:
> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
>> This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
>> ("UBIFS: Add security.* XATTR support for the UBIFS").
>
> Hi Richard,
> What about a full reverting of this commit. In ubifs, we
> *can* support any namespace of xattr including user, trusted, security
> or other anyone prefixed by any words. But we have a check_namespace()
> in xattr.c to limit what we want to support. That said, if we want to
> "Add security.* XATTR support for the UBIFS", what we need to do is
> just extending the check_namespace() to allow security namespace pass.
> And yes, check_namespace() have been supporting security namespace.
You're right. I thought/hoped we can re-use some parts of it.
Se let's do a full revert. I'll send a v2 patch in a jiffy.
> So, IMHO, we do not depend on the generic mechanism at all, and we can
> fully revert this commit.
>
> But strange to me, why we picked this commit for ubifs? Artem, is there
> something I am missing?
TBH, I fear nobody noticed. :(
Thanks,
//richard
On 08/20/2015 02:42 PM, Richard Weinberger wrote:
> Yang, (Sorry if I've used your last name lately)
Haha, that's fine. My friends in China all call me Dongsheng. :)
>
> Am 20.08.2015 um 05:00 schrieb Dongsheng Yang:
>> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
>>> Currently UBIFS does not support direct IO, but some applications
>>> blindly use the O_DIRECT flag.
>>> Instead of failing upon open() we can do better and fall back
>>> to buffered IO.
>>
>> Hmmmm, to be honest, I am not sure we have to do it as Dave
>> suggested. I think that's just a work-around for current fstests.
>>
>> IMHO, perform a buffered IO when user request direct IO without
>> any warning sounds not a good idea. Maybe adding a warning would
>> make it better.
>
> Well, how would you inform the user?
> A printk() to dmesg is useless are the vast majority of open()
> callers do not check dmesg... :)
>
> Major filesystems implement ->direct_IO these days and having
> a "return 0"-stub seems to be legit.
> For example exofs does too. So, I really don't consider it a work around.
Hmmm, then I am okey with this idea now.
>
>> I think we need more discussion about AIO&DIO in ubifs, and actually
>> I have a plan for it. But I have not listed the all cons and pros of
>> it so far.
>
> Sure, having a real ->direct_IO would be be best solution.
> My patch won't block this.
Yes, agree. So let's return 0 currently.
Yang
>
> Thanks,
> //richard
> .
>
On Wed, 2015-08-19 at 22:35 +0200, Richard Weinberger wrote:
> Currently UBIFS does not support direct IO, but some applications
> blindly use the O_DIRECT flag.
> Instead of failing upon open() we can do better and fall back
> to buffered IO.
>
> Cc: Dongsheng Yang <[email protected]>
> Cc: [email protected]
> Suggested-by: Dave Chinner <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
Richard,
The idea was to explicitly reject what we do not support. Let's say I
am an app which requires O_DIRECT, and which does not want to work with
non-O_DIRECT. What would I do to ensure O_DIRECT?
Could you please check what other file-systems which do not support
O_DIRECT do in this case? Do they also fall-back to normal IO instead
of explicitly failing? If yes, we can do what is considered to be the
"standard" behavior.
Thanks!
On Thu, 2015-08-20 at 11:00 +0800, Dongsheng Yang wrote:
> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> > Currently UBIFS does not support direct IO, but some applications
> > blindly use the O_DIRECT flag.
> > Instead of failing upon open() we can do better and fall back
> > to buffered IO.
>
> Hmmmm, to be honest, I am not sure we have to do it as Dave
> suggested. I think that's just a work-around for current fstests.
>
> IMHO, perform a buffered IO when user request direct IO without
> any warning sounds not a good idea. Maybe adding a warning would
> make it better.
>
> I think we need more discussion about AIO&DIO in ubifs, and actually
> I have a plan for it. But I have not listed the all cons and pros of
> it so far.
>
> Artem, what's your opinion?
Yes, this is my worry too.
Basically, we need to see what is the "common practice" here, and
follow it. This requires a small research. What would be the most
popular Linux FS which does not support direct I/O? Can we check what
it does?
Artem.
Artem,
Am 20.08.2015 um 13:31 schrieb Artem Bityutskiy:
> On Thu, 2015-08-20 at 11:00 +0800, Dongsheng Yang wrote:
>> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
>>> Currently UBIFS does not support direct IO, but some applications
>>> blindly use the O_DIRECT flag.
>>> Instead of failing upon open() we can do better and fall back
>>> to buffered IO.
>>
>> Hmmmm, to be honest, I am not sure we have to do it as Dave
>> suggested. I think that's just a work-around for current fstests.
>>
>> IMHO, perform a buffered IO when user request direct IO without
>> any warning sounds not a good idea. Maybe adding a warning would
>> make it better.
>>
>> I think we need more discussion about AIO&DIO in ubifs, and actually
>> I have a plan for it. But I have not listed the all cons and pros of
>> it so far.
>>
>> Artem, what's your opinion?
>
> Yes, this is my worry too.
>
> Basically, we need to see what is the "common practice" here, and
> follow it. This requires a small research. What would be the most
> popular Linux FS which does not support direct I/O? Can we check what
> it does?
All popular filesystems seem to support direct IO.
That's the problem, application do not expect O_DIRECT to fail.
My intention was to do it like exofs:
commit d83c7eb65d9bf0a57e7d5ed87a5bd8e5ea6b1fb6
Author: Boaz Harrosh <[email protected]>
Date: Mon Jan 13 23:45:43 2014 +0200
exofs: Allow O_DIRECT open
With this minimal do nothing patch an application can open O_DIRECT
and then actually do buffered sync IO instead. But the aio API is
supported which is a good thing
Signed-off-by: Boaz Harrosh <[email protected]>
diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index a52a5d2..7e7ba9a 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -961,6 +961,14 @@ static void exofs_invalidatepage(struct page *page, unsigned int offset,
WARN_ON(1);
}
+
+ /* TODO: Should be easy enough to do proprly */
+static ssize_t exofs_direct_IO(int rw, struct kiocb *iocb,
+ const struct iovec *iov, loff_t offset, unsigned long nr_segs)
+{
+ return 0;
+}
+
const struct address_space_operations exofs_aops = {
.readpage = exofs_readpage,
.readpages = exofs_readpages,
@@ -974,7 +982,7 @@ const struct address_space_operations exofs_aops = {
/* Not implemented Yet */
.bmap = NULL, /* TODO: use osd's OSD_ACT_READ_MAP */
- .direct_IO = NULL, /* TODO: Should be trivial to do */
+ .direct_IO = exofs_direct_IO,
/* With these NULL has special meaning or default is not exported */
.get_xip_mem = NULL,
Thanks,
//richard
On Thu, 2015-08-20 at 13:40 +0200, Richard Weinberger wrote:
> > Basically, we need to see what is the "common practice" here, and
> > follow it. This requires a small research. What would be the most
> > popular Linux FS which does not support direct I/O? Can we check
> > what
> > it does?
>
> All popular filesystems seem to support direct IO.
> That's the problem, application do not expect O_DIRECT to fail.
>
> My intention was to do it like exofs:
Fair enough, thanks!
Signed-off-by: Artem Bityutskiy <[email protected]>
Pardon the innocent bystander's comment, but:
On Thu, Aug 20, 2015 at 01:40:02PM +0200, Richard Weinberger wrote:
> Am 20.08.2015 um 13:31 schrieb Artem Bityutskiy:
> > On Thu, 2015-08-20 at 11:00 +0800, Dongsheng Yang wrote:
> >> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> >>> Currently UBIFS does not support direct IO, but some applications
> >>> blindly use the O_DIRECT flag.
> >>> Instead of failing upon open() we can do better and fall back
> >>> to buffered IO.
> >>
> >> Hmmmm, to be honest, I am not sure we have to do it as Dave
> >> suggested. I think that's just a work-around for current fstests.
> >>
> >> IMHO, perform a buffered IO when user request direct IO without
> >> any warning sounds not a good idea. Maybe adding a warning would
> >> make it better.
> >>
> >> I think we need more discussion about AIO&DIO in ubifs, and actually
> >> I have a plan for it. But I have not listed the all cons and pros of
> >> it so far.
> >>
> >> Artem, what's your opinion?
> >
> > Yes, this is my worry too.
> >
> > Basically, we need to see what is the "common practice" here, and
> > follow it. This requires a small research. What would be the most
> > popular Linux FS which does not support direct I/O? Can we check what
> > it does?
>
> All popular filesystems seem to support direct IO.
> That's the problem, application do not expect O_DIRECT to fail.
Why can't we just suggest fixing the applications? The man pages for
O_DIRECT clearly document the EINVAL return code:
EINVAL The filesystem does not support the O_DIRECT flag. See NOTES
for more information.
and under NOTES:
O_DIRECT support was added under Linux in kernel version 2.4.10.
Older Linux kernels simply ignore this flag. Some filesystems may not
implement the flag and open() will fail with EINVAL if it is used.
Brian
On Thu, 2015-08-20 at 13:49 -0700, Brian Norris wrote:
> Pardon the innocent bystander's comment, but:
>
> On Thu, Aug 20, 2015 at 01:40:02PM +0200, Richard Weinberger wrote:
> > Am 20.08.2015 um 13:31 schrieb Artem Bityutskiy:
> > > On Thu, 2015-08-20 at 11:00 +0800, Dongsheng Yang wrote:
> > > > On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> > > > > Currently UBIFS does not support direct IO, but some
> > > > > applications
> > > > > blindly use the O_DIRECT flag.
> > > > > Instead of failing upon open() we can do better and fall back
> > > > > to buffered IO.
> > > >
> > > > Hmmmm, to be honest, I am not sure we have to do it as Dave
> > > > suggested. I think that's just a work-around for current
> > > > fstests.
> > > >
> > > > IMHO, perform a buffered IO when user request direct IO without
> > > > any warning sounds not a good idea. Maybe adding a warning
> > > > would
> > > > make it better.
> > > >
> > > > I think we need more discussion about AIO&DIO in ubifs, and
> > > > actually
> > > > I have a plan for it. But I have not listed the all cons and
> > > > pros of
> > > > it so far.
> > > >
> > > > Artem, what's your opinion?
> > >
> > > Yes, this is my worry too.
> > >
> > > Basically, we need to see what is the "common practice" here, and
> > > follow it. This requires a small research. What would be the most
> > > popular Linux FS which does not support direct I/O? Can we check
> > > what
> > > it does?
> >
> > All popular filesystems seem to support direct IO.
> > That's the problem, application do not expect O_DIRECT to fail.
>
> Why can't we just suggest fixing the applications? The man pages for
> O_DIRECT clearly document the EINVAL return code:
>
> EINVAL The filesystem does not support the O_DIRECT flag. See NOTES
> for more information.
>
> and under NOTES:
>
> O_DIRECT support was added under Linux in kernel version 2.4.10.
> Older Linux kernels simply ignore this flag. Some filesystems may
> not
> implement the flag and open() will fail with EINVAL if it is used.
That's an option.
When writing the code, we were defensive and preferred to error out for
everything we do not support. This is generally a good SW engineering
practice.
Now, some user-space fails when direct I/O is not supported. We can
chose to fake direct I/O or fix user-space. The latter seems to be the
preferred course of actions, and you are correctly pointing the man
page.
However, if
1. we are the only FS erroring out on O_DIRECT
2. other file-systems not supporting direct IO just fake it
we may just follow the crowd and fake it too.
I am kind of trusting Richard here - I assume he did the research and
the above is the case, this is why I am fine with his patch.
Does this logic seem acceptable to you? Other folk's opinion would be
great to hear.
Artem.
On 08/24/2015 03:18 PM, Artem Bityutskiy wrote:
> On Thu, 2015-08-20 at 15:34 +0300, Artem Bityutskiy wrote:
>> On Thu, 2015-08-20 at 13:40 +0200, Richard Weinberger wrote:
>>>> Basically, we need to see what is the "common practice" here, and
>>>> follow it. This requires a small research. What would be the most
>>>> popular Linux FS which does not support direct I/O? Can we check
>>>> what
>>>> it does?
>>>
>>> All popular filesystems seem to support direct IO.
>>> That's the problem, application do not expect O_DIRECT to fail.
>>>
>>> My intention was to do it like exofs:
>>
>> Fair enough, thanks!
>>
>> Signed-off-by: Artem Bityutskiy <[email protected]>
>
> Richard, you mention this was suggested by Dave, could you please pint
> to the discussion, if possible?
http://lists.infradead.org/pipermail/linux-mtd/2015-August/060702.html
That's in a discussion I want to introduce ubifs into xfstests.
Yang
>
> Artem.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> .
>
On Thu, 2015-08-20 at 15:34 +0300, Artem Bityutskiy wrote:
> On Thu, 2015-08-20 at 13:40 +0200, Richard Weinberger wrote:
> > > Basically, we need to see what is the "common practice" here, and
> > > follow it. This requires a small research. What would be the most
> > > popular Linux FS which does not support direct I/O? Can we check
> > > what
> > > it does?
> >
> > All popular filesystems seem to support direct IO.
> > That's the problem, application do not expect O_DIRECT to fail.
> >
> > My intention was to do it like exofs:
>
> Fair enough, thanks!
>
> Signed-off-by: Artem Bityutskiy <[email protected]>
Richard, you mention this was suggested by Dave, could you please pint
to the discussion, if possible?
Artem.
On 08/24/2015 03:17 PM, Dongsheng Yang wrote:
> On 08/24/2015 03:18 PM, Artem Bityutskiy wrote:
>> On Thu, 2015-08-20 at 15:34 +0300, Artem Bityutskiy wrote:
>>> On Thu, 2015-08-20 at 13:40 +0200, Richard Weinberger wrote:
>>>>> Basically, we need to see what is the "common practice" here, and
>>>>> follow it. This requires a small research. What would be the most
>>>>> popular Linux FS which does not support direct I/O? Can we check
>>>>> what
>>>>> it does?
>>>>
>>>> All popular filesystems seem to support direct IO.
>>>> That's the problem, application do not expect O_DIRECT to fail.
>>>>
>>>> My intention was to do it like exofs:
>>>
>>> Fair enough, thanks!
>>>
>>> Signed-off-by: Artem Bityutskiy <[email protected]>
>>
>> Richard, you mention this was suggested by Dave, could you please pint
>> to the discussion, if possible?
>
> http://lists.infradead.org/pipermail/linux-mtd/2015-August/060702.html
>
> That's in a discussion I want to introduce ubifs into xfstests.
And Artem, I cc-ed that discussion to you, maybe you can find it in your
Inbox.
>
> Yang
>>
>> Artem.
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-fsdevel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> .
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> .
>
On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
> 1. we are the only FS erroring out on O_DIRECT
> 2. other file-systems not supporting direct IO just fake it
There are lots of file systems not supporting O_DIRECT, but ubifs might
be the most common one. Given that O_DIRECT implementations aren't
hard, so what's holding back a real implementation?
On 08/24/2015 04:03 PM, Christoph Hellwig wrote:
> On Mon, Aug 24, 2015 at 11:02:42AM +0300, Artem Bityutskiy wrote:
>> Back when we were writing UBIFS, we did not need direct IO, so we did
>> not implement it. But yes, probably someone who cares could just try
>> implementing this feature.
>
> So I think the answer here is to implement a real version insted of
> adding hacks to pretend it is supported.
Actually, I had a plan for it. But it's for 4.4 or 4.5 in my plan.
Yang
> .
>
On Mon, 2015-08-24 at 00:53 -0700, Christoph Hellwig wrote:
> On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
> > 1. we are the only FS erroring out on O_DIRECT
> > 2. other file-systems not supporting direct IO just fake it
>
> There are lots of file systems not supporting O_DIRECT, but ubifs
> might
> be the most common one. Given that O_DIRECT implementations aren't
> hard, so what's holding back a real implementation?
Back when we were writing UBIFS, we did not need direct IO, so we did
not implement it. But yes, probably someone who cares could just try
implementing this feature.
Artem.
On Mon, Aug 24, 2015 at 11:02:42AM +0300, Artem Bityutskiy wrote:
> Back when we were writing UBIFS, we did not need direct IO, so we did
> not implement it. But yes, probably someone who cares could just try
> implementing this feature.
So I think the answer here is to implement a real version insted of
adding hacks to pretend it is supported.
On Mon, Aug 24, 2015 at 03:17:10PM +0800, Dongsheng Yang wrote:
> >Richard, you mention this was suggested by Dave, could you please pint
> >to the discussion, if possible?
>
> http://lists.infradead.org/pipermail/linux-mtd/2015-August/060702.html
>
> That's in a discussion I want to introduce ubifs into xfstests.
FYI, xfstests is a test suite, _not_ an application. Adding O_DIRECT
just for xfstests is utterly dumb and suggeting that is even dumber.
xfstests should check for supported features, and skip tests that use
it.
On Mon, 2015-08-24 at 01:03 -0700, Christoph Hellwig wrote:
> On Mon, Aug 24, 2015 at 11:02:42AM +0300, Artem Bityutskiy wrote:
> > Back when we were writing UBIFS, we did not need direct IO, so we
> > did
> > not implement it. But yes, probably someone who cares could just
> > try
> > implementing this feature.
>
> So I think the answer here is to implement a real version insted of
> adding hacks to pretend it is supported.
Fair enough, thank for the input!
Artem.
Am 24.08.2015 um 11:34 schrieb Artem Bityutskiy:
> On Mon, 2015-08-24 at 01:03 -0700, Christoph Hellwig wrote:
>> On Mon, Aug 24, 2015 at 11:02:42AM +0300, Artem Bityutskiy wrote:
>>> Back when we were writing UBIFS, we did not need direct IO, so we
>>> did
>>> not implement it. But yes, probably someone who cares could just
>>> try
>>> implementing this feature.
>>
>> So I think the answer here is to implement a real version insted of
>> adding hacks to pretend it is supported.
>
> Fair enough, thank for the input!
Agreed. Let's drop this patch. :)
Thanks,
//richard
On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
> Now, some user-space fails when direct I/O is not supported.
I think the whole argument rested on what it means when "some user space
fails"; apparently that "user space" is just a test suite (which
can/should be fixed).
> We can
> chose to fake direct I/O or fix user-space. The latter seems to be the
> preferred course of actions, and you are correctly pointing the man
> page.
>
> However, if
>
> 1. we are the only FS erroring out on O_DIRECT
> 2. other file-systems not supporting direct IO just fake it
>
> we may just follow the crowd and fake it too.
>
> I am kind of trusting Richard here - I assume he did the research and
> the above is the case, this is why I am fine with his patch.
>
> Does this logic seem acceptable to you? Other folk's opinion would be
> great to hear.
Could work for me, though that doesn't seem ideal. Anyway, it now seems
Christopher and Richard agree with me.
Regards,
Brian
Brian Norris <[email protected]> writes:
> On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
>> Now, some user-space fails when direct I/O is not supported.
>
> I think the whole argument rested on what it means when "some user space
> fails"; apparently that "user space" is just a test suite (which
> can/should be fixed).
Even if it wasn't a test suite it should still fail. Either the fs
supports O_DIRECT or it doesn't. Right now, the only way an application
can figure this out is to try an open and see if it fails. Don't break
that.
Cheers,
Jeff
On Mon, Aug 24, 2015 at 01:19:24PM -0400, Jeff Moyer wrote:
> Brian Norris <[email protected]> writes:
>
> > On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
> >> Now, some user-space fails when direct I/O is not supported.
> >
> > I think the whole argument rested on what it means when "some user space
> > fails"; apparently that "user space" is just a test suite (which
> > can/should be fixed).
>
> Even if it wasn't a test suite it should still fail. Either the fs
> supports O_DIRECT or it doesn't. Right now, the only way an application
> can figure this out is to try an open and see if it fails. Don't break
> that.
Who cares how a filesystem implements O_DIRECT as long as it does
not corrupt data? ext3 fell back to buffered IO in many situations,
yet the only complaints about that were performance. IOWs, it's long been
true that if the user cares about O_DIRECT *performance* then they
have to be careful about their choice of filesystem.
But if it's only 5 lines of code per filesystem to support O_DIRECT
*correctly* via buffered IO, then exactly why should userspace have
to jump through hoops to explicitly handle open(O_DIRECT) failure?
Especially when you consider that all they can do is fall back to
buffered IO themselves....
Cheers,
Dave.
--
Dave Chinner
[email protected]
On Tue, Aug 25, 2015 at 09:46:11AM +1000, Dave Chinner wrote:
> On Mon, Aug 24, 2015 at 01:19:24PM -0400, Jeff Moyer wrote:
> > Brian Norris <[email protected]> writes:
> >
> > > On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
> > >> Now, some user-space fails when direct I/O is not supported.
> > >
> > > I think the whole argument rested on what it means when "some user space
> > > fails"; apparently that "user space" is just a test suite (which
> > > can/should be fixed).
> >
> > Even if it wasn't a test suite it should still fail. Either the fs
> > supports O_DIRECT or it doesn't. Right now, the only way an application
> > can figure this out is to try an open and see if it fails. Don't break
> > that.
>
> Who cares how a filesystem implements O_DIRECT as long as it does
> not corrupt data? ext3 fell back to buffered IO in many situations,
> yet the only complaints about that were performance. IOWs, it's long been
> true that if the user cares about O_DIRECT *performance* then they
> have to be careful about their choice of filesystem.
>
> But if it's only 5 lines of code per filesystem to support O_DIRECT
> *correctly* via buffered IO, then exactly why should userspace have
> to jump through hoops to explicitly handle open(O_DIRECT) failure?
> Especially when you consider that all they can do is fall back to
> buffered IO themselves....
This is what btrfs already does for O_DIRECT plus compressed, or other
cases where people don't want their applications to break on top of new
features that aren't quite compatible with it.
-chris
Dave Chinner <[email protected]> writes:
> On Mon, Aug 24, 2015 at 01:19:24PM -0400, Jeff Moyer wrote:
>> Brian Norris <[email protected]> writes:
>>
>> > On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
>> >> Now, some user-space fails when direct I/O is not supported.
>> >
>> > I think the whole argument rested on what it means when "some user space
>> > fails"; apparently that "user space" is just a test suite (which
>> > can/should be fixed).
>>
>> Even if it wasn't a test suite it should still fail. Either the fs
>> supports O_DIRECT or it doesn't. Right now, the only way an application
>> can figure this out is to try an open and see if it fails. Don't break
>> that.
>
> Who cares how a filesystem implements O_DIRECT as long as it does
> not corrupt data? ext3 fell back to buffered IO in many situations,
> yet the only complaints about that were performance. IOWs, it's long been
> true that if the user cares about O_DIRECT *performance* then they
> have to be careful about their choice of filesystem.
> But if it's only 5 lines of code per filesystem to support O_DIRECT
> *correctly* via buffered IO, then exactly why should userspace have
> to jump through hoops to explicitly handle open(O_DIRECT) failure?
> Especially when you consider that all they can do is fall back to
> buffered IO themselves....
I had written counterpoints for all of this, but I thought better of
it. Old versions of the kernel simply ignore O_DIRECT, so clearly
there's precedent.
I do think we should at least document what file systems appear to be
doing. Here's a man page patch for open (generated with extra context
for easier reading). Let me know what you think.
Cheers,
Jeff
p.s. I still think it's the wrong way to go, as it makes it harder for
an admin to determine what is actually going on.
diff --git a/man2/open.2 b/man2/open.2
index 06c0a29..acc438b 100644
--- a/man2/open.2
+++ b/man2/open.2
@@ -1471,17 +1471,18 @@ a flag of the same name, but without alignment restrictions.
.LP
.B O_DIRECT
support was added under Linux in kernel version 2.4.10.
Older Linux kernels simply ignore this flag.
Some filesystems may not implement the flag and
.BR open ()
will fail with
.B EINVAL
-if it is used.
+if it is used. Other file systems may implement O_DIRECT via
+buffered I/O, which is essentially the same as ignoring the flag.
.LP
Applications should avoid mixing
.B O_DIRECT
and normal I/O to the same file,
and especially to overlapping byte regions in the same file.
Even when the filesystem correctly handles the coherency issues in
this situation, overall I/O throughput is likely to be slower than
using either mode alone.
On Tue, Aug 25, 2015 at 10:00:58AM -0400, Jeff Moyer wrote:
> Dave Chinner <[email protected]> writes:
>
> > On Mon, Aug 24, 2015 at 01:19:24PM -0400, Jeff Moyer wrote:
> >> Brian Norris <[email protected]> writes:
> >>
> >> > On Mon, Aug 24, 2015 at 10:13:25AM +0300, Artem Bityutskiy wrote:
> >> >> Now, some user-space fails when direct I/O is not supported.
> >> >
> >> > I think the whole argument rested on what it means when "some user space
> >> > fails"; apparently that "user space" is just a test suite (which
> >> > can/should be fixed).
> >>
> >> Even if it wasn't a test suite it should still fail. Either the fs
> >> supports O_DIRECT or it doesn't. Right now, the only way an application
> >> can figure this out is to try an open and see if it fails. Don't break
> >> that.
> >
> > Who cares how a filesystem implements O_DIRECT as long as it does
> > not corrupt data? ext3 fell back to buffered IO in many situations,
> > yet the only complaints about that were performance. IOWs, it's long been
> > true that if the user cares about O_DIRECT *performance* then they
> > have to be careful about their choice of filesystem.
>
> > But if it's only 5 lines of code per filesystem to support O_DIRECT
> > *correctly* via buffered IO, then exactly why should userspace have
> > to jump through hoops to explicitly handle open(O_DIRECT) failure?
>
> > Especially when you consider that all they can do is fall back to
> > buffered IO themselves....
>
> I had written counterpoints for all of this, but I thought better of
> it. Old versions of the kernel simply ignore O_DIRECT, so clearly
> there's precedent.
>
> I do think we should at least document what file systems appear to be
> doing. Here's a man page patch for open (generated with extra context
> for easier reading). Let me know what you think.
We shouldn't be ignoring it, but instead call it similar to O_DSYNC plus
removing the pages from cache.
-chris
Chris Mason <[email protected]> writes:
>> I do think we should at least document what file systems appear to be
>> doing. Here's a man page patch for open (generated with extra context
>> for easier reading). Let me know what you think.
>
> We shouldn't be ignoring it, but instead call it similar to O_DSYNC plus
> removing the pages from cache.
Ah, right. I'll fix that up, thanks.
-Jeff
On Mon, 2015-08-24 at 21:28 -0400, Chris Mason wrote:
> This is what btrfs already does for O_DIRECT plus compressed, or
> other
> cases where people don't want their applications to break on top of
> new
> features that aren't quite compatible with it.
I do not know how much of direct IO we can do in UBIFS - we have
compression too, there is not block layer under us. But someone should
try and see what could be done.
Artem.
On Thu, Aug 20, 2015 at 10:48:38AM +0800, Dongsheng Yang wrote:
> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> >This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
> >("UBIFS: Add security.* XATTR support for the UBIFS").
>
> Hi Richard,
> What about a full reverting of this commit. In ubifs, we
> *can* support any namespace of xattr including user, trusted, security
> or other anyone prefixed by any words. But we have a check_namespace()
> in xattr.c to limit what we want to support. That said, if we want to
> "Add security.* XATTR support for the UBIFS", what we need to do is
> just extending the check_namespace() to allow security namespace pass.
> And yes, check_namespace() have been supporting security namespace.
Is this good enough? Yes, it'd mean that the xattrs end up on disk, but
then who's responsible for invoking the selected LSMs inode_init_security() hooks?
AFAICT, we'd still need to invoke security_inode_init_security for newly
created inodes (which, Richard's proposed commit still does).
Thanks,
Josh (who, admittedly, is neither a filesystem nor security module guy :)
On 08/26/2015 10:15 PM, Josh Cartwright wrote:
> On Thu, Aug 20, 2015 at 10:48:38AM +0800, Dongsheng Yang wrote:
>> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
>>> This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
>>> ("UBIFS: Add security.* XATTR support for the UBIFS").
>>
>> Hi Richard,
>> What about a full reverting of this commit. In ubifs, we
>> *can* support any namespace of xattr including user, trusted, security
>> or other anyone prefixed by any words. But we have a check_namespace()
>> in xattr.c to limit what we want to support. That said, if we want to
>> "Add security.* XATTR support for the UBIFS", what we need to do is
>> just extending the check_namespace() to allow security namespace pass.
>> And yes, check_namespace() have been supporting security namespace.
>
> Is this good enough? Yes, it'd mean that the xattrs end up on disk, but
> then who's responsible for invoking the selected LSMs inode_init_security() hooks?
> AFAICT, we'd still need to invoke security_inode_init_security for newly
> created inodes (which, Richard's proposed commit still does).
OH, right. My bad!!!! I missed the security_inode_init_security().
Besides to allow security.* prefix in xattr, we still need to call
security_inode_init_security() in ubifs_create(). That's true.
So what we need to remove is only the ubifs_xattr_handlers.
Thanx Josh, you are right.
And Richard, sorry for my bad mind.
Reviewed-by: Dongsheng Yang <[email protected]>
Thanx
Yang
>
> Thanks,
>
> Josh (who, admittedly, is neither a filesystem nor security module guy :)
>