Re: [PATCHv2 01/15] fs: configfs: Add unlocked version of configfs_depend_item()

[Andrzej Pietrasiewicz]

W dniu 26.10.2015 o 16:35, Krzysztof Opasiak pisze:


Hi Christoph,






I have just updated this patch but it turned out that to applying your
remarks I should split this into a few commits. That's why I decided to
send it as a separate series[1].

@Andrzej
Could you please rebase your series onto top of my v3 [1] and resend it
without my patch?



Rebased onto v4 ;) and resent.

Andrzej
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 01/15] fs: configfs: Add unlocked version of configfs_depend_item()

[Krzysztof Opasiak]

Hi Christoph,

On 10/23/2015 04:51 PM, Andrzej Pietrasiewicz wrote:

W dniu 22.10.2015 o 17:22, Christoph Hellwig pisze:

Hi Andrzej,

please find a way to share code between the two depend function.  And
also drop the duplicate undepend version and just stop passing the
unused subsystem argument.  Not only do we not keep unused argument in
Linux in general, but in this case it's also really useful for the new
API.


As far as I know Krzysztof will submit the patch again early next week,
addressing the issues you mention.



I have just updated this patch but it turned out that to applying your 
remarks I should split this into a few commits. That's why I decided to 
send it as a separate series[1].


@Andrzej
Could you please rebase your series onto top of my v3 [1] and resend it 
without my patch?


Footnotes:
1 - https://www.mail-archive.com/linux-usb@vger.kernel.org/msg65871.html

Thank you,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 01/15] fs: configfs: Add unlocked version of configfs_depend_item()

[Andrzej Pietrasiewicz]

W dniu 22.10.2015 o 17:22, Christoph Hellwig pisze:

Hi Andrzej,

please find a way to share code between the two depend function.  And
also drop the duplicate undepend version and just stop passing the
unused subsystem argument.  Not only do we not keep unused argument in
Linux in general, but in this case it's also really useful for the new
API.


As far as I know Krzysztof will submit the patch again early next week,
addressing the issues you mention.

AP

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 01/15] fs: configfs: Add unlocked version of configfs_depend_item()

[Christoph Hellwig]

Hi Andrzej,

please find a way to share code between the two depend function.  And
also drop the duplicate undepend version and just stop passing the
unused subsystem argument.  Not only do we not keep unused argument in
Linux in general, but in this case it's also really useful for the new
API.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 01/15] fs: configfs: Add unlocked version of configfs_depend_item()

[Andrzej Pietrasiewicz]

From: Krzysztof Opasiak 

This change is necessary for the SCSI target usb gadget composed with
configfs. In this case configfs will be used for two different purposes:
to compose a usb gadget and to configure the target part. If an instance
of tcm function is created in $CONFIGFS_ROOT/usb_gadget//functions
a tpg can be created in $CONFIGFS_ROOT/target/usb_gadget//, but after
a tpg is created the tcm function must not be removed until its
corresponding tpg is gone. While the configfs_depend/undepend_item() are
meant exactly for creating this kind of dependencies, they are not suitable
if the other kernel subsystem happens to be another subsystem in configfs,
so this patch adds unlocked versions meant for configfs callbacks.

In configfs_depend_item() we have to consider two possible cases:

1) When we are called to depend another item in the same subsystem as
caller: In this case we should skip locking configfs root as we know that
configfs is in valid state and our subsystem will not be unregistered
during this call.

2) When we are called to depend item in different subsystem than our caller
In this case we are also sure that configfs is in valid state but   we have
to lock root of configfs to avoid unregistration of target's subsystem.
As it is other than caller's subsystem, there may be nothing what protects
us against unregistration of that subsystem.

Signed-off-by: Krzysztof Opasiak 
Signed-off-by: Andrzej Pietrasiewicz 
---
 fs/configfs/dir.c| 97 
 include/linux/configfs.h | 12 ++
 2 files changed, 109 insertions(+)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index c81ce7f..2578ac0 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1152,6 +1152,103 @@ void configfs_undepend_item(struct configfs_subsystem 
*subsys,
 }
 EXPORT_SYMBOL(configfs_undepend_item);
 
+/*
+ * caller_subsys is a caller's subsystem not target's. This is used to
+ * determine if we should lock root and check subsys or not. When we are
+ * in the same subsystem as our target there is no need to do locking as
+ * we know that subsys is valid and is not unregistered during this function
+ * as we are called from callback of one of his children and VFS holds a lock
+ * on some inode. Otherwise we have to lock our root to  ensure that target's
+ * subsystem it is not unregistered during this function.
+ */
+int configfs_depend_item_unlocked(struct configfs_subsystem *caller_subsys,
+ struct config_item *target)
+{
+   struct configfs_subsystem *target_subsys;
+   struct config_group *root, *parent;
+   struct configfs_dirent *sd, *subsys_sd = NULL;
+   int ret = -ENOENT;
+
+   /* Disallow this function for configfs root */
+   if (configfs_is_root(target))
+   return -EINVAL;
+
+   parent = target->ci_group;
+   /*
+* This may happen when someone is trying to depend root
+* directory of some subsystem
+*/
+   if (configfs_is_root(>cg_item)) {
+   target_subsys = to_configfs_subsystem(to_config_group(target));
+   root = parent;
+   } else {
+   target_subsys = parent->cg_subsys;
+   /* Find a cofnigfs root as we may need it for locking */
+   for (root = parent; !configfs_is_root(>cg_item);
+root = root->cg_item.ci_group)
+   ;
+   }
+
+   if (target_subsys != caller_subsys) {
+   struct configfs_dirent *p, *root_sd;
+   /*
+* We are in other configfs subsystem, so we have to do
+* additional locking to prevent other subsystem from being
+* unregistered
+*/
+   mutex_lock(_inode(root->cg_item.ci_dentry)->i_mutex);
+
+   /*
+* As we are trying to depend item from other subsystem
+* we have to check if this subsystem is still registered
+*/
+   root_sd = root->cg_item.ci_dentry->d_fsdata;
+   list_for_each_entry(p, _sd->s_children, s_sibling) {
+   if (p->s_type & CONFIGFS_DIR &&
+   _subsys->su_group.cg_item == p->s_element) {
+   subsys_sd = p;
+   break;
+   }
+   }
+
+   if (!subsys_sd)
+   goto out_root_unlock;
+   } else {
+   subsys_sd = target_subsys->su_group.cg_item.ci_dentry->d_fsdata;
+   }
+
+   spin_lock(_dirent_lock);
+   ret = configfs_depend_prep(subsys_sd->s_dentry, target);
+   if (ret)
+   goto out_unlock;
+
+   sd = target->ci_dentry->d_fsdata;
+   sd->s_dependent_count += 1;
+
+   ret = 0;
+
+out_unlock:
+   spin_unlock(_dirent_lock);
+
+   if